Skip to content

bpo-33021: Release the GIL during fstat() calls #6019

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 11, 2018

Conversation

nirs
Copy link
Contributor

@nirs nirs commented Mar 7, 2018

fstat may block for long time if the file descriptor is on a
non-responsive NFS server, hanging all threads. Most fstat() calls are
handled by _Py_fstat(), releasing the GIL internally, but but
_Py_fstat_noraise() does not release the GIL, and most calls release the
GIL explicitly around it.

This patch fixes last 2 calls to _Py_fstat_no_raise(), avoiding hangs
when calling:

  • mmap.mmap()
  • os.urandom()
  • random.seed()

https://bugs.python.org/issue33021

fstat may block for long time if the file descriptor is on a
non-responsive NFS server, hanging all threads. Most fstat() calls are
handled by _Py_fstat(), releasing the GIL internally, but but
_Py_fstat_noraise() does not release the GIL, and most calls release the
GIL explicitly around it.

This patch fixes last 2 calls to _Py_fstat_no_raise(), avoiding hangs
when calling:
- mmap.mmap()
- os.urandom()
- random.seed()
nirs added a commit to nirs/cpython that referenced this pull request Mar 7, 2018
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()
@nirs
Copy link
Contributor Author

nirs commented Mar 11, 2018

@vstinner, can you review this?

@pitrou
Copy link
Member

pitrou commented Mar 11, 2018

Hi @nirs, sorry for the delay. @vstinner is currently unavailable, so I'm gonna review this.

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. The changes look basically fine to me. The only thing missing is a NEWS entry with a short description of the fix. You should use the blurb tool to generate it.

@bedevere-bot
Copy link

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@nirs
Copy link
Contributor Author

nirs commented Mar 11, 2018

Thanks for the review @pitrou! I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

@pitrou
Copy link
Member

pitrou commented Mar 11, 2018

Thank you @nirs !

@miss-islington
Copy link
Contributor

Thanks @nirs for the PR, and @pitrou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @nirs for the PR, and @pitrou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-6159 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 20, 2018
fstat may block for long time if the file descriptor is on a
non-responsive NFS server, hanging all threads. Most fstat() calls are
handled by _Py_fstat(), releasing the GIL internally, but but
_Py_fstat_noraise() does not release the GIL, and most calls release the
GIL explicitly around it.

This patch fixes last 2 calls to _Py_fstat_no_raise(), avoiding hangs
when calling:
- mmap.mmap()
- os.urandom()
- random.seed()
(cherry picked from commit 4484f9d)

Co-authored-by: Nir Soffer <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 20, 2018
fstat may block for long time if the file descriptor is on a
non-responsive NFS server, hanging all threads. Most fstat() calls are
handled by _Py_fstat(), releasing the GIL internally, but but
_Py_fstat_noraise() does not release the GIL, and most calls release the
GIL explicitly around it.

This patch fixes last 2 calls to _Py_fstat_no_raise(), avoiding hangs
when calling:
- mmap.mmap()
- os.urandom()
- random.seed()
(cherry picked from commit 4484f9d)

Co-authored-by: Nir Soffer <[email protected]>
@bedevere-bot
Copy link

GH-6160 is a backport of this pull request to the 3.6 branch.

pitrou pushed a commit that referenced this pull request Mar 20, 2018
)

fstat may block for long time if the file descriptor is on a
non-responsive NFS server, hanging all threads. Most fstat() calls are
handled by _Py_fstat(), releasing the GIL internally, but but
_Py_fstat_noraise() does not release the GIL, and most calls release the
GIL explicitly around it.

This patch fixes last 2 calls to _Py_fstat_no_raise(), avoiding hangs
when calling:
- mmap.mmap()
- os.urandom()
- random.seed()
(cherry picked from commit 4484f9d)

Co-authored-by: Nir Soffer <[email protected]>
pitrou pushed a commit that referenced this pull request Mar 20, 2018
)

fstat may block for long time if the file descriptor is on a
non-responsive NFS server, hanging all threads. Most fstat() calls are
handled by _Py_fstat(), releasing the GIL internally, but but
_Py_fstat_noraise() does not release the GIL, and most calls release the
GIL explicitly around it.

This patch fixes last 2 calls to _Py_fstat_no_raise(), avoiding hangs
when calling:
- mmap.mmap()
- os.urandom()
- random.seed()
(cherry picked from commit 4484f9d)

Co-authored-by: Nir Soffer <[email protected]>
jo2y pushed a commit to jo2y/cpython that referenced this pull request Mar 23, 2018
fstat may block for long time if the file descriptor is on a
non-responsive NFS server, hanging all threads. Most fstat() calls are
handled by _Py_fstat(), releasing the GIL internally, but but
_Py_fstat_noraise() does not release the GIL, and most calls release the
GIL explicitly around it.

This patch fixes last 2 calls to _Py_fstat_no_raise(), avoiding hangs
when calling:
- mmap.mmap()
- os.urandom()
- random.seed()
@nirs nirs deleted the fstat-gil-master branch October 1, 2022 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants