test_pickle/test_pickletools fail when running tests sequentially.

[Yhg1s]

When running the test suite sequentially (as is done as part of the release process), an interaction between test_imp, test_importlib and subinterpreters is causing test_pickle and test_pickletools to fail. The easiest reproducer::

% ./python -m test test_imp test_importlib test_pickletools
0:00:00 load avg: 0.22 Run tests sequentially
0:00:00 load avg: 0.22 [1/3] test_imp
0:00:00 load avg: 0.22 [2/3] test_importlib
0:00:03 load avg: 0.22 [3/3] test_pickletools
test test_pickletools failed -- Traceback (most recent call last):
  File "Lib/test/pickletester.py", line 1989, in test_builtin_types
    s = self.dumps(t, proto)
        ^^^^^^^^^^^^^^^^^^^^
  File "Lib/test/test_pickletools.py", line 11, in dumps
    return pickletools.optimize(pickle.dumps(arg, proto, **kwargs))
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
_pickle.PicklingError: Can't pickle <class 'importlib._bootstrap.BuiltinImporter'>: it's not the same object as importlib._bootstrap.BuiltinImporter

test_pickletools failed (1 error)

== Tests result: FAILURE ==

2 tests OK.

1 test failed:
    test_pickletools

Total duration: 4.2 sec
Tests result: FAILURE

The failure happens in both test_pickle and test_pickletools (test_pickletools's traceback is more obvious), after in the same process running both test_imp and either test_import or test_importlib. Disabling test_imp.ImportTests.test_create_builtin_subinterp() makes the tests pass. (This was happening before #102982 as well as after it, so I don't think it is involved here.)

I expect this is an unintentional side-effect of the test, but this was not happening in the 3.12.0a6 release, and as far as I can tell the relevant tests haven't changed since before a6. I'm delaying the 3.12.0a7 release until it's clear this isn't a fundamental problem with subinterpreters.

Linked PRs

• GH-103247: bypass the import cache on the _require_loader helper #104186
• gh-103247: add tearDown to test_loader and clear module cache #104226
• [3.11] gh-103247: clear the module cache in a test in test_importlib/extensions/test_loader.py (GH-104226) #104345

[Yhg1s]

The problem is actually reproducible all the way back to 3.12.0a4, even though it was never caught in the release testing (I don't know why, yet). The problem was exposed by #100142, but that merely adds testing of picklability of builtins, which all things considered is not an unreasonable test.

[Yhg1s]

As it appears this is merely uncovering a long standing issue with a hole in subinterpreter isolation and importlib (the same test combination fails in 3.10 and 3.11), I'm unblocking a7 for now, but leaving this as a release blocker for beta 1.

[erlend-aasland]

I did some bisecting in 3.10 and 3.11, and it landed like this:

• In 3.10, first bad commit: 37dbbb2 ([3.10] gh-99578: Fix refleak in _imp.create_builtin() (GH-99642) #99644)
• In 3.11, first bad commit: 9dda902 ([3.11] gh-99578: Fix refleak in _imp.create_builtin() (GH-99642) #99643)

Strangely, I do not get the same result in main; the "parent" commit (cb2ef8b, gh-99642) that those backports were created from, reproduces the failure, as expected, but cb2ef8b2acbb231c207207d3375b2f8b0077a6ee~ (1cae31d) is not a "good" commit; the failure also occurs there.
The first bad commit I get in main is:

• 1f369ad (gh-98354: Add unicode check for 'name' attribute in _imp_create_builtin #98412)

The linked issues for those PRs contain some interesting history and debugging:

• gc_decref: Assertion "gc_get_refs(g) > 0" failed #99578
• create_builtin() in _imp module trigger segfault if taking a builtin object as input #98354

(It would be nice if someone else could bisect on their machine and verify my findings.)

[sunmy2019]

even though it was never caught in the release testing (I don't know why, yet).

Is it because the release testing in running in 4 processes?

[Yhg1s]

No, the release testing is done sequentially, and that hasn't changed in 2 years: https://github.com/python/release-tools/blame/master/run_release.py#L486

[FFY00]

Just some research for others also looking into it.

The failure comes from test_builtin_types, which is supposed to test pickling the builtins, but to do so it iterates over builtins.__dict__.values(), which includes all module attributes, including __loader__, which is what ultimately causes the failure here. IMO we should probably be using dir(builtins) instead.

cpython/Lib/test/pickletester.py

Lines 1985 to 1990 in fd1947e

	def test_builtin_types(self):
	for t in builtins.__dict__.values():
	if isinstance(t, type) and not issubclass(t, BaseException):
	for proto in protocols:
	s = self.dumps(t, proto)
	self.assertIs(self.loads(s), t)

I suspect the issue is simply that test_importlib leaks some of its monkeypatching, causing modules in other tests to be imported by a copy of the importlib module, hence the pickle identity check failure. In a normal situation, the loader should come from the frozen version of importlib, so that's a good hint.

cpython/Lib/test/test_importlib/util.py

Lines 52 to 53 in 52bc2e7

	source = import_helper.import_fresh_module(module_name, fresh=fresh,
	blocked=('_frozen_importlib', '_frozen_importlib_external'))

The deal with test_imp is that both test_issue98354 and test_create_builtin_subinterp import the builtins module, causing it to be imported by a copy of the importlib module, as explained above, and cached. If you disable both of those tests, the failure is gone, but if you keep either, the pickle tests fail.

I think this is simply caused by leakage of the importlib tests, and has nothing to do with subinterpreters.

[sunmy2019]

I am testing on with macos 13.3 clang 14

commit b98d2d31bffcaeb0c4c8848a8d1b35419c70b2da
Author: Serhiy Storchaka <storchaka@gmail.com>
Date:   Wed Dec 21 16:31:22 2022 +0200

    gh-100129: Add tests for pickling all builtin types and functions (GH-100142)

 Lib/test/pickletester.py | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

FWIW, I discovered that, test_importlib has many submodules.

You can invoke test as

./python -m test test_imp {t} test_pickletools

with different t, such as t = "test_importlib.builtin.test_finder"

We got a map from t to returncode.

{
    "test_importlib.builtin.test_finder": 2,
    "test_importlib.builtin.test_loader": 2,
    "test_importlib.extension.test_case_sensitivity": 2,
    "test_importlib.extension.test_finder": 2,
    "test_importlib.extension.test_loader": 2,
    "test_importlib.frozen.test_finder": 2,
    "test_importlib.frozen.test_loader": 2,
    "test_importlib.import_.test___loader__": 2,
    "test_importlib.import_.test___package__": 2,
    "test_importlib.import_.test_api": 2,
    "test_importlib.import_.test_caching": 2,
    "test_importlib.import_.test_fromlist": 2,
    "test_importlib.import_.test_helpers": 2,
    "test_importlib.import_.test_meta_path": 2,
    "test_importlib.import_.test_packages": 2,
    "test_importlib.import_.test_path": 2,
    "test_importlib.import_.test_relative_imports": 2,
    "test_importlib.resources.test_compatibilty_files": 0,
    "test_importlib.resources.test_contents": 0,
    "test_importlib.resources.test_files": 0,
    "test_importlib.resources.test_open": 0,
    "test_importlib.resources.test_path": 0,
    "test_importlib.resources.test_read": 0,
    "test_importlib.resources.test_reader": 0,
    "test_importlib.resources.test_resource": 0,
    "test_importlib.source.test_case_sensitivity": 2,
    "test_importlib.source.test_finder": 2,
    "test_importlib.source.test_source_encoding": 2,
    "test_importlib.test_abc": 2,
    "test_importlib.test_api": 2,
    "test_importlib.test_lazy": 2,
    "test_importlib.test_locks": 2,
    "test_importlib.test_main": 0,
    "test_importlib.test_metadata_api": 0,
    "test_importlib.test_namespace_pkgs": 2,
    "test_importlib.test_pkg_import": 0,
    "test_importlib.test_spec": 2,
    "test_importlib.test_threaded_import": 0,
    "test_importlib.test_util": 2,
    "test_importlib.test_zip": 0,
}

[sunmy2019]

Looks like "those files that returned code 2" all import Lib/test/test_importlib/util.py?

Hope it is helpful.

[ericsnowcurrently]

IMO we should probably be using dir(builtins) instead.

FWIW, at the very least we should skip the module-specific dunder names (__name__, __doc__, __spec__, __loader__, __package__).

That said, we should also make sure there's a test somewhere that catches the reported failure (which we happened to get lucky catching in the pickle tests).

I suspect the issue is simply that test_importlib leaks some of its monkeypatching,

+1 (but worth verifying)

I think this is simply caused by leakage of the importlib tests, and has nothing to do with subinterpreters.

+1 (but worth verifying)

[JelleZijlstra]

test_imp is no more, so the original reproduction steps no longer work. However, I see this failure on current main (no pickle tests necessary):

% ./python.exe -m test test_importlib test_import
Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 1.91 Run tests sequentially
0:00:00 load avg: 1.91 [1/2] test_importlib
0:00:10 load avg: 2.29 [2/2] test_import
test test_import failed -- Traceback (most recent call last):
  File "/Users/jelle/py/cpython/Lib/test/test_import/__init__.py", line 1815, in test_multi_init_extension_compat
    require_extension(module)
  File "/Users/jelle/py/cpython/Lib/test/test_import/__init__.py", line 90, in require_extension
    _require_loader(module, ExtensionFileLoader, skip)
  File "/Users/jelle/py/cpython/Lib/test/test_import/__init__.py", line 76, in _require_loader
    actual = MODULE_KINDS[actual]
             ~~~~~~~~~~~~^^^^^^^^
KeyError: <class 'importlib._bootstrap_external.ExtensionFileLoader'>

test_import failed (1 error)

== Tests result: FAILURE ==

1 test OK.

1 test failed:
    test_import

Total duration: 13.5 sec
Tests result: FAILURE

[sunmy2019]

test_imp is no more, so the original reproduction steps no longer work. However, I see this failure on current main (no pickle tests necessary):

I looked into it. The failure (changing the env) comes from

cpython/Lib/test/test_importlib/extension/test_loader.py

Lines 263 to 273 in e5b8b19

	def test_try_registration(self):
	# Assert that the PyState_{Find,Add,Remove}Module C API doesn't work.
	module = self.load_module()
	with self.subTest('PyState_FindModule'):
	self.assertEqual(module.call_state_registration_func(0), None)
	with self.subTest('PyState_AddModule'):
	with self.assertRaises(SystemError):
	module.call_state_registration_func(1)
	with self.subTest('PyState_RemoveModule'):
	with self.assertRaises(SystemError):
	module.call_state_registration_func(2)

Commenting on it solves this issue.

It seems not to follow the with util.uncache(self.name) idiom in adjacent tests. (I did not check the code logic, just did A/B test)

[FFY00]

Good catch! Do you want to open a PR with the fix?

I think it'd make sense to add a teardown method which unloads self.name instead of relying on util.uncache to be used in all tests.

[sunmy2019]

Good catch! Do you want to open a PR with the fix?

#104226