Some fstat() calls do not release the GIL, possibly hanging all threads

[nirs]

BPO	33021
Nosy	@Yhg1s, @brettcannon, @ncoghlan, @nirs, @pitrou, @vstinner, @rbtcollins, @pmp-p, @ericsnowcurrently, @serhiy-storchaka, @1st1, @MojoVampire, @ZackerySpytz
PRs	bpo-33021: Release the GIL during fstat() calls #6019 [2.7] bpo-33021: Release the GIL during fstat() calls (GH-6019) #6020 bpo-33021: Fix GCC 7 warning (-Wmaybe-uninitialized) in mmapmodule.c #6117 [3.7] bpo-33021: Release the GIL during fstat() calls (GH-6019) #6159 [3.6] bpo-33021: Release the GIL during fstat() calls (GH-6019) #6160
Files	mmap_nfs_test.py mmap_size_nfs_test.py fdopen_nfs_test.py

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2018-03-20.19:40:42.831>
created_at = <Date 2018-03-07.23:03:24.256>
labels = ['3.8', '3.7', 'library', 'performance']
title = 'Some fstat() calls do not release the GIL, possibly hanging all threads'
updated_at = <Date 2018-03-20.19:40:42.831>
user = 'https://github.com/nirs'

bugs.python.org fields:

activity = <Date 2018-03-20.19:40:42.831>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2018-03-20.19:40:42.831>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2018-03-07.23:03:24.256>
creator = 'nirs'
dependencies = []
files = ['47490', '47491', '47492']
hgrepos = []
issue_num = 33021
keywords = ['patch']
message_count = 17.0
messages = ['313407', '313409', '313417', '313419', '313434', '313627', '313836', '313837', '313839', '313980', '313981', '313982', '314024', '314083', '314158', '314159', '314161']
nosy_count = 13.0
nosy_names = ['twouters', 'brett.cannon', 'ncoghlan', 'nirs', 'pitrou', 'vstinner', 'rbcollins', 'pmpp', 'eric.snow', 'serhiy.storchaka', 'yselivanov', 'josh.r', 'ZackerySpytz']
pr_nums = ['6019', '6020', '6117', '6159', '6160']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue33021'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

[nirs]

If the file descriptor is on a non-responsive NFS server, calling
fstat() can block for long time, hanging all threads. Most of the fstat()
calls release the GIL around the call, but some calls seems to be
forgotten.

In python 3, the calls are handled now by _py_fstat(), releasing the GIL
internally, but some calls use _py_fstat_noraise() which does not release
the GIL. Most of the calls to _py_fstat_noraise() release the GIL around
the call, except these 2 calls, affecting users of:

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

In python there are more fstat() calls to fix, affecting users of:

• imp.load_dynamic()
• imp.load_source()
• mmap.mmap()
• mmapobject.size()
• os.fdopen()
• os.urandom()
• random.seed()

[pmp-p]

is fstat thread safe ?

[MojoVampire]

fstat is async signal safe, and I suspect it's thread safe in general, though usage does rely on the file descriptor remaining valid. If the fd in question is closed in another thread after the GIL is released, fstat would fail; if a new file is opened and assigned the same fd, it would give erroneous results. But I don't think any of that would lead to fundamentally incorrect behavior (after all, you could do the same thing manually by caching an fd then closing the file).

[pmp-p]

josh.r. i think you are right, i was worried if a nfs sillyrename is in progress, for eg a lock file ,then server hangs but thread lift the GIL and allow another thread to try to start to fstat the same path.

[nirs]

Python cannot protect raw file descriptor from bad multi-threaded
application. For example the application may close a file descriptor twice
which may lead to closing unrelated file descriptor created by another
thread just after it was closed, before the second close.

This issue affects all function using raw file descriptors, and we cannot
protect them with the GIL.

Even if fstat was not thread safe, we cannot protect it using the GIl
since this blocks the entire application until fstat returns.

[pitrou]

New changeset 4484f9d by Antoine Pitrou (Nir Soffer) in branch 'master':
bpo-33021: Release the GIL during fstat() calls (GH-6019)
4484f9d

[ZackerySpytz]

Commit 4484f9d causes GCC 7.2.0 to emit a warning.

cpython/Modules/mmapmodule.c: In function ‘new_mmap_object’:
cpython/Modules/mmapmodule.c:1126:18: warning: ‘fstat_result’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     if (fd != -1 && fstat_result == 0 && S_ISREG(status.st_mode)) {

A PR is attached.

[pitrou]

New changeset d6e1404 by Antoine Pitrou (Zackery Spytz) in branch 'master':
bpo-33021: Fix GCC 7 warning (-Wmaybe-uninitialized) in mmapmodule.c (bpo-6117)
d6e1404

[pitrou]

With Benjamin we've decided that this wouldn't happen in 2.7. Performance improvements don't warrant a backport.

Thank you Nir for reporting and posting the patches!