Possible thread-unsafe initialization of wrapped modules?

[ngoldbaum]

Consider the following script:

from concurrent.futures import ThreadPoolExecutor
from pkgutil import walk_packages
import scipy

def worker():
    for _ in walk_packages(scipy.__path__, scipy.__name__ + '.'):
        pass

n_threads=10

tpe = ThreadPoolExecutor(max_workers=min((n_threads, 4)))
futures = [None]*n_threads


for i in range(n_threads):
    futures[i] = tpe.submit(worker)

[f.result() for f in futures]

This is based on the scipy test scipy/_lib/tests/test_public_api.py::test_all_modules_are_expected running under pytest-run-parallel.

On both the free-threaded and GIL-enabled interpreter, this script eventually fails with the following error:

Traceback (most recent call last):
  File "/Users/goldbaum/Documents/test/test.py", line 16, in <module>
    [f.result() for f in futures]
     ~~~~~~~~^^
  File "/Users/goldbaum/.pyenv/versions/3.13.4/lib/python3.13/concurrent/futures/_base.py", line 456, in result
    return self.__get_result()
           ~~~~~~~~~~~~~~~~~^^
  File "/Users/goldbaum/.pyenv/versions/3.13.4/lib/python3.13/concurrent/futures/_base.py", line 401, in __get_result
    raise self._exception
  File "/Users/goldbaum/.pyenv/versions/3.13.4/lib/python3.13/concurrent/futures/thread.py", line 59, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Users/goldbaum/Documents/test/test.py", line 6, in worker
    for _ in walk_packages(scipy.__path__, scipy.__name__ + '.'):
             ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/goldbaum/.pyenv/versions/3.13.4/lib/python3.13/pkgutil.py", line 93, in walk_packages
    yield from walk_packages(path, info.name+'.', onerror)
  File "/Users/goldbaum/.pyenv/versions/3.13.4/lib/python3.13/pkgutil.py", line 93, in walk_packages
    yield from walk_packages(path, info.name+'.', onerror)
  File "/Users/goldbaum/.pyenv/versions/3.13.4/lib/python3.13/pkgutil.py", line 93, in walk_packages
    yield from walk_packages(path, info.name+'.', onerror)
  File "/Users/goldbaum/.pyenv/versions/3.13.4/lib/python3.13/pkgutil.py", line 88, in walk_packages
    path = getattr(sys.modules[info.name], '__path__', None) or []
                   ~~~~~~~~~~~^^^^^^^^^^^
KeyError: 'scipy._lib.array_api_compat.dask.array'

It runs successfully if I set n_threads=1 in the script.

I think this is happening because there's a race to call clone_module:

array-api-compat/array_api_compat/_internal.py

Lines 56 to 71 in 6c708d1

	def clone_module(mod_name: str, globals_: dict[str, object]) -> list[str]:
	"""Import everything from module, updating globals().
	Returns __all__.
	"""
	mod = importlib.import_module(mod_name)
	# Neither of these two methods is sufficient by itself,
	# depending on various idiosyncrasies of the libraries we're wrapping.
	objs = {}
	exec(f"from {mod.__name__} import *", objs)
	for n in dir(mod):
	if not n.startswith("_") and hasattr(mod, n):
	objs[n] = getattr(mod, n)
	globals_.update(objs)
	return list(objs)

[ngoldbaum]

I think this is happening because sys.modules can change "underneath" a thread - it's even documented to do this: https://docs.python.org/3/library/sys.html#sys.modules, and CPython directly using sys.modules in pkgutil.walk_packages is unsafe for shim modules like the ones provided by array_api_compat. If the shim fails to import because e.g. dask isn't installed, then the shim module might be transiently visible to other threads as available to import. Because the implementation of walk_packages does sys.modules[info.name], if info.name is only transiently available, the dict access might fail. If I modify CPython to do sys.modules.get(info.name, None), then I can avoid this failure mode.

Now I'm not completely sure this is correct, because there is supposed to be a per-module import lock that is held by the interpreter during import - doesn't that protect us here?

[seberg]

there is supposed to be a per-module import lock that is held by the interpreter during import - doesn't that protect us here?

Randomly saw this part and it reminded me of this: python/cpython#91238 where I am still confused if we should add locking in NumPy or CPython should make sure to protect us.

[ev-br]

I am still confused if we should add locking in NumPy or CPython should make sure to protect us.

Seconded. I'm confused too. Naively, it feels like CPython should protect us here.