Note: This may be a dupe or a generalization of 595601.




Running below code snippet on 2.3.1 release and debug 
build on Windows 2000/XP a few times will inevitably lead 
to a crash. 2.2.2 also exhibits this behavior.




The crashing code:




------------


import thread




f=open("tmp1", "w")




def worker():


    global f


    while 1:


        f.close()


        f=open("tmp1", "w")


        f.seek (0, 0)




thread.start_new_thread(worker, ())


thread.start_new_thread(worker, ())


while 1: pass


-------------




The issue appears to be this (and similar) code sections 
from fileobject.c:




	Py_BEGIN_ALLOW_THREADS


	errno = 0;


	ret = _portable_fseek(f->f_fp, offset, whence);


	Py_END_ALLOW_THREADS




Note that due to 
Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS, 
f->f_fp can be set to NULL by file_close prior to entering 
_portable_fseek. Similar crashes can be observed with 
read, write and close itself when they are mixed with a 
concurrent close.




Obviously, the offending python code is buggy and a lock 
should be used to prevent concurrent access to f. 
However, it seems unreasonable for Python to crash 
because of it.




A mutex preventing concurrent access to the file object 
seems like a reasonable way to fix this.

Logged In: YES 
user_id=29957

Attaching the failing code as an attachment, because SF
mangles source code.

Logged In: YES 
user_id=877914

My apologies for the mangled source. I suppose there isn't a 
way for me to remedy the situation.

Logged In: YES 
user_id=21627

janixia, don't worry about the formatting - this is
partially SF's fault, too.

Would you be interested in developing and testing a patch? I
think it would be sufficient to move the f->f_fp access out
of the GIL releasage.

Logged In: YES 
user_id=31392

Patch 595601 is an attempt to address this problem, but it's
incomplete.  The file object API allows an extension to
extract to FILE * and squirrel it away.  That's clearly
unsafe, because it can't participate in a locking scheme
without re-writing extensions.

Shane Hathaway proposed another solution here:
http://mail.python.org/pipermail/python-dev/2003-June/036543.html

The problem in this case is that we cause the call to
close() to raise an exception.  I'd prefer to see the
exception raised elsewhere, because close() seldom fails and
is often closed from routines that are cleaning up at the
end.  On the other hand, this solution would be easier to
implementation, so I'm at least +0 on it.

Let's do one or the other.

Logged In: YES 
user_id=877914

I'm inclined to go with Shane's suggested solution of 
reference counting when file access is in progress. It requires 
no synchronisation (increment, decrement and check are 
outside global lock release) and should have the smallest 
performance impact.

I don't think the FILE * extraction problem can be solved at 
all. Once the horse it out of the barn... However, for 
the "standard" case Shane's suggestion provides a neat and 
clean solution for the problem.

If the community can agree on this solution, I could be talked 
into implementing it.

If I read the 2003 python-dev thready correctly, there isn't a solution
to this.  Does this need to go back to python-dev, or do we just call it
"wont fix"?  Or...?

I'm still able to reproduce the bug in Python 2.5 (svn) and 2.6 (trunk).

import thread
f=open("tmp1", "w")

def worker():
    global f
    while 1:
        f.close()
        f=open("tmp1", "w")
        f.seek(0,0)

thread.start_new_thread(worker, ())
thread.start_new_thread(worker, ())

Unhandled exception in thread started by <function worker at 0xb7d01aac>
Traceback (most recent call last):
*** glibc detected *** ./python: malloc(): memory corruption: 0xb7efc008 ***
======= Backtrace: =========
/lib/tls/i686/cmov/libc.so.6[0xb7dbe636]
/lib/tls/i686/cmov/libc.so.6(__libc_malloc+0x90)[0xb7dbffc0]
/lib/tls/i686/cmov/libc.so.6[0xb7dad03f]
/lib/tls/i686/cmov/libc.so.6(fopen64+0x2c)[0xb7daf61c]
./python(PyTraceBack_Print+0x1a4)[0x80ef0f4]
./python(PyErr_Display+0x76)[0x80e73a6]
./python[0x80ed80d]
./python(PyObject_Call+0x27)[0x805c927]
./python(PyEval_CallObjectWithKeywords+0x6c)[0x80c151c]
./python(PyErr_PrintEx+0xbe)[0x80e7e9e]
./python[0x80f37b1]
/lib/tls/i686/cmov/libpthread.so.0[0xb7ed146b]
/lib/tls/i686/cmov/libc.so.6(clone+0x5e)[0xb7e276de]
======= Memory map: ========
08048000-0813d000 r-xp 00000000 fe:01 10586072  
/home/heimes/dev/python/release25-maint/python
0813d000-08162000 rw-p 000f4000 fe:01 10586072  
/home/heimes/dev/python/release25-maint/python
08162000-081fe000 rw-p 08162000 00:00 0          [heap]
b6a00000-b6a21000 rw-p b6a00000 00:00 0
b6a21000-b6b00000 ---p b6a21000 00:00 0
b6bc1000-b6bc2000 ---p b6bc1000 00:00 0
b6bc2000-b73c2000 rw-p b6bc2000 00:00 0
b73c2000-b73c3000 ---p b73c2000 00:00 0
b73c3000-b7bc3000 rw-p b73c3000 00:00 0
b7bc3000-b7bff000 r-xp 00000000 08:05 325941     /lib/libncurses.so.5.6
b7bff000-b7c07000 rw-p 0003b000 08:05 325941     /lib/libncurses.so.5.6
b7c07000-b7c4e000 r-xp 00000000 08:05 325837     /lib/libncursesw.so.5.6
b7c4e000-b7c56000 rw-p 00046000 08:05 325837     /lib/libncursesw.so.5.6
b7c56000-b7c82000 r-xp 00000000 08:05 325955     /lib/libreadline.so.5.2
b7c82000-b7c86000 rw-p 0002c000 08:05 325955     /lib/libreadline.so.5.2
b7c86000-b7c87000 rw-p b7c86000 00:00 0
b7c87000-b7c8a000 r-xp 00000000 fe:01 10716611  
/home/heimes/dev/python/release25-maint/build/lib.linux-i686-2.5/readline.so
b7c8a000-b7c8b000 rw-p 00003000 fe:01 10716611  
/home/heimes/dev/python/release25-maint/build/lib.linux-i686-2.5/readline.so
b7c8b000-b7c92000 r--s 00000000 08:05 557857    
/usr/lib/gconv/gconv-modules.cache
b7c92000-b7cd1000 r--p 00000000 08:05 570306    
/usr/lib/locale/de_DE.utf8/LC_CTYPE
b7cd1000-b7d54000 rw-p b7cd1000 00:00 0
b7d54000-b7e98000 r-xp 00000000 08:05 326311    
/lib/tls/i686/cmov/libc-2.6.1.so
b7e98000-b7e99000 r--p 00143000 08:05 326311    
/lib/tls/i686/cmov/libc-2.6.1.so
b7e99000-b7e9b000 rw-p 00144000 08:05 326311    
/lib/tls/i686/cmov/libc-2.6.1.so
b7e9b000-b7e9e000 rw-p b7e9b000 00:00 0
b7e9e000-b7ec1000 r-xp 00000000 08:05 326315    
/lib/tls/i686/cmov/libm-2.6.1.so
b7ec1000-b7ec3000 rw-p 00023000 08:05 326315    
/lib/tls/i686/cmov/libm-2.6.1.so
b7ec3000-b7ec5000 r-xp 00000000 08:05 326330    
/lib/tls/i686/cmov/libutil-2.6.1.so
b7ec5000-b7ec7000 rw-p 00001000 08:05 326330    
/lib/tls/i686/cmov/libutil-2.6.1.so
b7ec7000-b7ec8000 rw-p b7ec7000 00:00 0
b7ec8000-b7eca000 r-xp 00000000 08:05 326314    
/lib/tls/i686/cmov/libdl-2.6.1.so
b7eca000-b7ecc000 rw-p 00001000 08:05 326314    
/lib/tls/i686/cmov/libdl-2.6.1.so
b7ecc000-b7ee0000 r-xp 00000000 08:05 326325    
/lib/tls/i686/cmov/libpthread-2.6.1.so
b7ee0000-b7ee2000 rw-p 00013000 08:05 326325    
/lib/tls/i686/cmov/libpthread-2.6.1.so
b7ee2000-b7ee4000 rw-p b7ee2000 00:00 0
b7ef1000-b7efb000 r-xp 00000000 08:05 325908     /lib/libgcc_s.so.1
b7efb000-b7efc000 rw-p 0000a000 08:05 325908     /lib/libgcc_s.so.1
b7efc000-b7f01000 rw-p b7efc000 00:00 0
b7f01000-b7f1b000 r-xp 00000000 08:05 326530     /lib/ld-2.6.1.so
b7f1b000-b7f1d000 rw-p 00019000 08:05 326530     /lib/ld-2.6.1.so
bfcd2000-bfcee000 rw-p bfcd2000 00:00 0          [stack]
ffffe000-fffff000 r-xp 00000000 00:00 0          [vdso]

However Python 3.0 doesn't crash:

Unhandled exception in thread started by <function worker at 0x840860c>
Traceback (most recent call last):
  File "<stdin>", line 6, in worker
  File "/home/heimes/dev/python/py3k/Lib/io.py", line 1234, in seek
    self.buffer.seek(pos)
  File "/home/heimes/dev/python/py3k/Lib/io.py", line 877, in seek
    return self.raw.seek(pos, whence)
IOError: [Errno 9] Bad file descriptor

A small addition to Christian's code snippet allows me to reproduce the
problem as well:

import thread

f=open("tmp1", "w")

def worker():
    global f
    while 1:
        f.close()
        f = open("tmp1", "w")
        f.seek(0,0) 

thread.start_new_thread(worker, ())
thread.start_new_thread(worker, ())

while 1:
    pass

This is a preliminary patch which shows how things might be done better.
It only addresses close(), seek() and dealloc right now. However, as
mentioned in test_close_open_seek, if I raise the number of workers, I
get crashes (while test_close_open is fine). Perhaps fseek() in the
glibc is thread unsafe when operating on the same file descriptor?

Another approach would be to add a dedicated lock for each PyFileObject.
This sounds a bit bad performance-wise but after all the GIL itself is a
lock, and we release and re-acquire it on each file operation, so why not?

On the other hand, surprisingly enough, the flockfile/funlockfile
manpage tells me that:

       The stdio functions are thread-safe.  This is achieved by 
assigning  to  each  FILE
       object  a  lockcount  and  (if the lockcount is nonzero) an
owning thread.  For each
       library call, these functions wait until the FILE object is no
longer  locked  by  a
       different thread, then lock it, do the requested I/O, and unlock
the object again.

This leaves me wondering what is happening in the above-mentioned test.

Why hadn't I read #595601 in detail, it has an explanation: 

[quoting Jeremy Hylton]
The universal newline code is squirrels the FILE * in a
local variable, which is worse.  If it happens that
another thread closes the file, at best the local
points to a closed FILE *.  But that memory could get
recycled and then there's no way to know what it points to.
[/quoting]

Even with careful coding, there's a small window between releasing the
GIL on our side, and acquiring the FILE-specific lock in the glibc,
during which the fclose() function can be invoked and release the FILE
just before we invoke another function (e.g. fseek()) on it.

Some good news: I've found a way to continue with my approach and make
it working. The close() method now raises an appropriate IOError when
another method is being called from another thread. Attaching a patch,
which protects a bunch of file object methods (together with tests). 

Now the bad news: the protection logic in fileobject.c is, hmm, a bit
contrived (and I'm not even sure it's 100% correct). If someone wants to
read it and put his veto before I go further...

Closed #595601 as a duplicate.

Actually, my approach was not 100% correct, it failed in some rare
cases. I've come to the conclusion that using an unlock count on the
PyFileObject is the correct way to proceed. I'm now attaching a working
and complete patch which protects all methods of the PyFileObject. The
original test suite runs fine, as well as the added test cases and Tim
Peters' crasher here:
http://mail.python.org/pipermail/python-dev/2003-June/036537.html

To sum up the changes brought by this patch:
- no supplementary locking
- but each time we release the GIL to do an operation on a FILE, we
increase a dedicated counter on the PyFileObject
- when close()ing a PyFileObject, if the aforementioned counter is
non-zero, we throw an IOError rather than risking calling fclose(). By
construction this cannot happen in the PyFileObject destructor, but if
ever it happens (for example if a C extension decides to put its hands
in the PyFileObject struct), we throw a SystemError instead.

Ah, I had forgotten to protect the print statement as well. Here is a
new patch :-)

I'm reviewing this patch now and plan to commit it after some testing.

A couple comments:

I'd rename your sts variables to status.

Also FYI:

Your use of volatile on the int unlocked_count member of PyFileObject
does not do what you think it does and isn't needed here anyways.

Access to the variable is always protected by the GIL unlocking and
locking of which should cause an implicit memory barrier guaranteeing
that all other CPUs in the system will see the same value stored in the
structure in memory.

The C volatile keyword on the other hand does not guarantee this. 
volatile is useful for memory mapped IO but it makes no guarantees about
cache coherent access between multiple CPUs.  (the atomic types in the
recent C++ standards are meant for that)


Both of the above are trivial changes, no need for another patch.

I've attached my patch that I want to commit.  The main change from
filethread4 is some cleanup in file_test to make it run a lot faster and
add verbose mode output to indicate how well it is actually testing the
problem (counting the times that close raises IOError).

One concern holding up my commit:

Will this test pass on windows?  It is opening and closing the same file
in 'w+' mode from multiple threads of the same process at once.

Can someone with a windows dev environment please apply this patch and
test it.  If it dislikes the above file behavior, can you propose a fix
for it (set windows file non-exclusive flags or whatever you're supposed
to do... the worse alternative would be to use a new filename on each
open but that could cause a nightmare of thousands of new files being
created by the test which then have to be cleaned up)?

thanks,
-gps

Patched and tested on one of my buildbots, test_file passes without 
error with your latest Patch Greg.

Ok Greg, I wasn't sure locking/unlocking the GIL would create a memory
barrier but it sounds logical after all. Your patch looks fine to me.

Committed to trunk in revision 62195.

Misc/NEWS entry added.

I also added two new C API functions: PyFile_IncUseCount and
PyFile_DecUseCount along with documentation.  They should be used by any
C extension code that uses PyFile_AsFile and wants to make use of the
returned FILE* with the GIL released.

The net effect of not using them is no change from the existing behavior
(crashes would be possible) for those C extension modules.

I know this is long closed, but no one on the nosy list happens to have
this fix backported to 2.5, do they? :) If not, I'll attach one here
eventually...

> I know this is long closed, but no one on the nosy list happens to have
> this fix backported to 2.5, do they? :)

I think that at the time no one was sure the patch was 100% harmless. It
also subtly changes the behaviour of close() when called while another
IO operation is in progress in another thread, which is arguably a bug
fix but can still raise an exception it wouldn't have raised in 2.5.

So all in all I'm not sure this should be backported, although it would
probably be an improvement in most cases. I'll let someone else take the
decision.

The fix can not be committed to Python 2.5 because it breaks
compatibility by adding another field to the PyFileObject struct and
adding two new C API functions.

0) I ran into some (small) problems with the documentation added by revision 62195. It seems more efficient to reuse this issue to report these. Feel free to ask me to open another issue if that's not appreciated.

1) A small patch that addresses two problems with the current (ie, 2.7) documentation should be attached:
- link three occurrences of "GIL" to the GIL entry in the glossary; and
- add some example code to clarify the usage of PyFile_IncUseCount() andPyFile_DecUseCount().

2) That patch also adds a link to the "Thread State and the Global Interpreter Lock" section to the GIL entry in the Documentation index. That is a separate problem. But fixing that minor problem could also be of help to people (like me) that need to better understand the GIL aspects of those two functions.

Paul, please open a new issue and attach your patch(s) there; it should address issues in 2.7 and 3.2 only.

> please open a new issue and attach your patch(s) there

Issue 10111 now tracks the documentation problems.