[2.7] bpo-33021: Release the GIL during fstat() calls (GH-6019)#6020
[2.7] bpo-33021: Release the GIL during fstat() calls (GH-6019)#6020nirs wants to merge 2 commits into
Conversation
If the file descriptor is on a non-responsive NFS server, calling fstat() can block for long time, hanging all threads. Most of the calls release the GIL around the call, but some calls seems to be forgotten. This patch fixes the last calls, affecting users of: - imp.load_dynamic() - imp.load_source() - mmap.mmap() - mmapobject.size() - os.fdopen() - os.urandom() - random.seed()
There was a problem hiding this comment.
As on the other PR, this looks good on the principle, but you'll need to generate a NEWS entry.
Also, since 2.7 is in maintenance mode, I'm cc'ing our release manager Benjamin to get his approval on this. @benjaminp
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
Thanks for the review @pitrou! I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @pitrou: please review the changes made to this pull request. |
|
Thanks for the update. This looks good on the principle. I'll defer to @benjaminp for the backport decision. |
|
Releasing the GIL in new places tends to open new and exciting opportunities for reëntrancy bugs, so I'm not sure this is completely safe. |
|
I'm not sure about reëntrancy bugs, but I agree there may be thread-safetÿ issues on ill-behaving platforms (though for fstat() we already release the GIL in other places). |
|
Actually, you're right, I remember about someone implementing a FUSE filesystem with Python and there were interesting reëntrancy issues with GIL releasing specifics. |
|
Ok, I'm closing this PR as rejected. |
|
@benjaminp, we are releasing the GIL on all fstat calls in python 3. Why is this correct and safe in python 3 but no in python 2? I'm pretty sure that not releasing the GIL is not safe. Try to mmap a file on non-responsive NFS server - do you really want the entire process to hang? I can extract the mmap.xxx and os.fdopen changes to a separate patch, since the other changes will are much less likely to block (e.g. fstat for /dev/urandom). |
|
I'm not sure throwing |
|
@benjaminp I suggest to continue on the issue, I added more info there. |
If the file descriptor is on a non-responsive NFS server, calling
fstat() can block for long time, hanging all threads. Most of the calls
release the GIL around the call, but some calls seems to be forgotten.
This patch fixes the last calls, affecting users of:
https://bugs.python.org/issue33021