C unpickling bypasses import thread safety

[tjb900]

BPO	34572
Nosy	@brettcannon, @ncoghlan, @pitrou, @avassalotti, @ericsnowcurrently, @tjb900, @gstarnberger
PRs	bpo-34572: change _pickle unpickling to use import rather than retrieving from sys.modules #9047 [3.7] bpo-34572: change _pickle unpickling to use import rather than retrieving from sys.modules (GH-9047) #11921
Files	reproducer_submit.py reproduce_34572.py find_class_deadlock.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 2019-02-18.15:53:12.514>
created_at = <Date 2018-09-03.15:53:21.527>
labels = ['extension-modules', '3.8', 'type-bug', '3.7']
title = 'C unpickling bypasses import thread safety'
updated_at = <Date 2019-11-21.19:24:51.077>
user = 'https://github.com/tjb900'

bugs.python.org fields:

activity = <Date 2019-11-21.19:24:51.077>
actor = 'Valentyn Tymofieiev'
assignee = 'none'
closed = True
closed_date = <Date 2019-02-18.15:53:12.514>
closer = 'pitrou'
components = ['Extension Modules']
creation = <Date 2018-09-03.15:53:21.527>
creator = 'tjb900'
dependencies = []
files = ['47784', '48715', '48736']
hgrepos = []
issue_num = 34572
keywords = ['patch', 'needs review']
message_count = 13.0
messages = ['324528', '327729', '335023', '335027', '335096', '335098', '335099', '335842', '335844', '335845', '356641', '356697', '357200']
nosy_count = 8.0
nosy_names = ['brett.cannon', 'ncoghlan', 'pitrou', 'alexandre.vassalotti', 'eric.snow', 'tjb900', 'Valentyn Tymofieiev', 'gst']
pr_nums = ['9047', '11921']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue34572'
versions = ['Python 3.7', 'Python 3.8']

[tjb900]

Retrieving and using a module directly from sys.modules (from C in this case) leads to a race condition where the module may be importing on another thread but has not yet been initialised. For slow filesystems or large modules (e.g. numpy) this seems to lead to easily reproducible errors (the attached code fails 100% of the time on my work machine - CentOS 7).

I believe they have to be in sys.modules during this phase due to the possibility of circular references.

importlib handles this carefully with locking, but _pickle.c bypasses all that, leading to issues with threaded codes that use pickling, e.g. dask/distributed.

[tjb900]

Hi! Just wondering if there is anything I can do to move this along?

[pitrou]

Interesting I could not reproduce here, even by throwing Pandas into the mix and spawning 1024 workers...

[ericsnowcurrently]

Perhaps PyImport_GetModule() should aquire-release the module's lock before the lookup? This would effectively be a call to _lock_unlock_module() in importlib._bootstrap.

The alternative is to encourage using PyImport_Import() instead, like the PR has done. In the case the docs for PyImport_GetModule() should make it clear that it is guaranteed that the module is fully imported yet (and recommend using PyImport_Import() for the guarantee).

Either way there should be a new issue for the more general change (and it should reference this issue).

[pitrou]

I agree that more generally PyImport_GetModule() should be fixed. But it should be done carefully so as to not to lose the performance benefit of doing it. I think we should open a separate issue about that.

PS: one possibility is to reuse the optimization already done in PyImport_ImportModuleLevelObject():

        /* Optimization: only call _bootstrap._lock_unlock_module() if
           __spec__._initializing is true.
           NOTE: because of this, initializing must be set *before*
           stuffing the new module in sys.modules.
         */
        spec = _PyObject_GetAttrId(mod, &PyId___spec__);
        if (_PyModuleSpec_IsInitializing(spec)) {
            PyObject *value = _PyObject_CallMethodIdObjArgs(interp->importlib,
                                            &PyId__lock_unlock_module, abs_name,
                                            NULL);
            if (value == NULL) {
                Py_DECREF(spec);
                goto error;
            }
            Py_DECREF(value);
        }
        Py_XDECREF(spec);

[pitrou]

Opened bpo-35943 for PyImport_GetModule().

[ericsnowcurrently]

Thanks, Antoine.

[pitrou]

New changeset 4371c0a by Antoine Pitrou (tjb900) in branch 'master':
bpo-34572: change _pickle unpickling to use import rather than retrieving from sys.modules (GH-9047)
4371c0a

[pitrou]

New changeset 3129432 by Antoine Pitrou (Miss Islington (bot)) in branch '3.7':
bpo-34572: change _pickle unpickling to use import rather than retrieving from sys.modules (GH-9047) (GH-11921)
3129432