Race condition when importing collections.abc

@davidhewitt

Bug report

Bug description:

Discovered alongside #125243 with similar steps to reproduce. I don't have a simpler way to trigger this than "run the PyO3 tests in a loop" because I think it requires many threads accessing the python runtime simulatenously.

To trigger it, have a rust toolchain and Python installed, clone the PyO3 repo and execute the following command:

while RUST_BACKTRACE=1 UNSAFE_PYO3_BUILD_FREE_THREADED=1 cargo test --lib --features=full -- --test-threads=100; do :; done

You may also hit some other test failures related to ZoneInfo, see the other issue I opened about that.

You will eventually see a test failure with the following text:

---- exceptions::socket::tests::gaierror stdout ----
thread 'exceptions::socket::tests::gaierror' panicked at src/impl_/exceptions.rs:26:17:
failed to import exception socket.gaierror: ImportError: cannot import name 'Mapping' from 'collections.abc' (/Users/goldbaum/.pyenv/versions/3.13.0t/lib/python3.13t/collections/abc.py)

I slightly modified PyO3 to get a traceback as well (hidden because it's a distractingly long diff):

Details

diff --git a/src/impl_/exceptions.rs b/src/impl_/exceptions.rs
index 15b6f53b..de63ad59 100644
--- a/src/impl_/exceptions.rs
+++ b/src/impl_/exceptions.rs
@@ -1,4 +1,8 @@
-use crate::{sync::GILOnceCell, types::PyType, Bound, Py, Python};
+use crate::{
+    sync::GILOnceCell,
+    types::{PyTracebackMethods, PyType},
+    Bound, Py, Python,
+};

 pub struct ImportedExceptionTypeObject {
     imported_value: GILOnceCell<Py<PyType>>,
@@ -20,8 +24,11 @@ impl ImportedExceptionTypeObject {
             .import(py, self.module, self.name)
             .unwrap_or_else(|e| {
                 panic!(
-                    "failed to import exception {}.{}: {}",
-                    self.module, self.name, e
+                    "failed to import exception {}.{}: {}\n{}",
+                    self.module,
+                    self.name,
+                    e,
+                    e.traceback(py).unwrap().format().unwrap(),
                 )
             })
     }

And the traceback is:

Traceback (most recent call last):
  File "/Users/goldbaum/.pyenv/versions/3.13.0t/lib/python3.13t/socket.py", line 55, in <module>
    import os, sys, io, selectors
  File "/Users/goldbaum/.pyenv/versions/3.13.0t/lib/python3.13t/selectors.py", line 10, in <module>
    from collections.abc import Mapping

So somehow during setup of the socket module, Mapping isn't available yet, but only if many threads are simultaneously touching the Python runtime.

(ping @davidhewitt, we probably want to disable the socket error tests on the free-threaded build as well)

CPython versions tested on:

3.13

Operating systems tested on:

macOS

Linked PRs

pythongh-125245: Fix ZoneInfo data race in free threading build

EDIT: Commented on wrong issue

@serhiy-storchaka

There's a race condition that I believe was introduced in #123613. The sys.modules[__name__] = _collections_abc is not safe with the longstanding importlib optimization that avoids _ModuleLockManager if the module is already initialized:

cpython/Lib/importlib/_bootstrap.py

Lines 1351 to 1355 in a726ce7

    
           # Optimization: we avoid unneeded module locking if the module 
        
           # already exists in sys.modules and is fully initialized. 
        
           module = sys.modules.get(name, _NEEDS_LOADING) 
        
           if (module is _NEEDS_LOADING or 
        
               getattr(getattr(module, "__spec__", None), "_initializing", False)):

I think this may affect the default (with GIL) build as well depending on when the GIL-switch happens, but it definitely will happen more often on the free threading build.

The problematic execution when two threads (T1, T2) run import collections.abc:

T1: Starts to import Lib/collections/abc.py, sets sys.modules["collections.abc"]
T2: Loads module = sys.modules["collections.abc"] (line 1353 above). This points to the Lib/collections/abc.py module.
T1: Finishes importing collections/abc.py, sys.modules["collections.abc"] now points to the Lib/_collections_abc.py module.
T2: Checks module. __spec__. _initializing (line 1355) on the . This module is finished initializing, so it's returned. Note that at this point sys.modules["collections.abc"] points to the correct _collections_abc.py module, but the local variable module still points to the collections/abc.py module.

T2 now incorrectly has the Python collections.abc module.

cc @serhiy-storchaka

EDIT: Lib/_collections_abc.py is also a Python module

I think we can do something like:

--- a/Lib/collections/__init__.py
+++ b/Lib/collections/__init__.py
@@ -29,6 +29,9 @@
 import _collections_abc
 import sys as _sys

+_sys.modules['collections.abc'] = _collections_abc
+abc = _collections_abc
+
 from itertools import chain as _chain
 from itertools import repeat as _repeat
 from itertools import starmap as _starmap
--- a/Lib/collections/abc.py
+++ /dev/null
@@ -1,3 +0,0 @@
-import _collections_abc
-import sys
-sys.modules[__name__] = _collections_abc

pythongh-125245: Fix race condition when importing `collections.abc`

I think this may affect the default (with GIL) build as well depending on when the GIL-switch happens, but it definitely will happen more often on the free threading build.

Indeed. We see this in Fedora with the default (with GIL) build as well.

This is fixed now.

There's an open PR, but it's not merged yet. I don't think the underlying issue is fixed.

I'm waiting on more feedback regarding the issue that Serhiy raised on the PR:

#125415 (review)

Oops, I'm sorry!

gh-125245: Fix race condition when importing `collections.abc` (#125415)

pythongh-125245: Fix race condition when importing `collections.abc` (p…

[3.13] gh-125245: Fix race condition when importing `collections.abc` (…

The PRs are merged now

pythongh-125245: Fix race condition when importing `collections.abc` (p…

Navigation Menu

Repository navigation

Race condition when importing `collections.abc` #125245

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

	# Optimization: we avoid unneeded module locking if the module
	# already exists in sys.modules and is fully initialized.
	module = sys.modules.get(name, _NEEDS_LOADING)
	if (module is _NEEDS_LOADING or
	getattr(getattr(module, "__spec__", None), "_initializing", False)):

Race condition when importing collections.abc #125245

Description

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

Activity

colesbury commented on Oct 10, 2024

colesbury commented on Oct 11, 2024

colesbury commented on Oct 11, 2024

hroncok commented on Oct 16, 2024

ngoldbaum commented on Oct 24, 2024

colesbury commented on Oct 24, 2024

ngoldbaum commented on Oct 24, 2024

colesbury commented on Oct 25, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

Race condition when importing `collections.abc` #125245