gh-117953: Work Relative to Specific Extension Kinds in the Import Machinery #118205

ericsnowcurrently · 2024-04-24T01:17:24Z

I've pulled this out of #118116.

This change will make some later changes simpler.

Issue: Single-Phase Init Extension Module Init Functions Still Run in Isolated Interpreters #117953

…-phase init.

ericsnowcurrently · 2024-04-29T19:40:46Z

@encukou, aside from the other things this PR does, it should also take care of the problem with is_singlephase() you described in #117953 (comment).

encukou

Thank you! I have a few comments of the nitpicky kind.

encukou · 2024-05-01T07:37:42Z

Python/import.c

+}
+
+static bool
+check_multiphase_preinit(PyModuleDef *def)


Nitpick: consider making these do the assert, and naming them assert_multiphase_preinit & assert_singlephase. “check” is ambigous (should it do the assert?), and there aren't really suitable for anything but asserts.

encukou · 2024-05-01T07:56:40Z

Python/import.c

+            // XXX Two modules should not share the same def->m_base.
+            //assert(def->m_base.m_init == NULL
+            //           || def->m_base.m_init == singlephase->m_init);


Hm, I don't remember why this is here.
Let's have the tests find where this was needed? A debug-build assert probably is the best tool.

Suggested change

// XXX Two modules should not share the same def->m_base.

//assert(def->m_base.m_init == NULL

// || def->m_base.m_init == singlephase->m_init);

// The following assert used to be skipped if

// def->m_base.m_init == singlephase->m_init

// but two modules should not share the same def->m_base.

assert(def->m_base.m_init == NULL);

Yeah, that isn't very clear and the commented-out assert isn't helping matters. I'll clarify the comment and drop the rest.

encukou · 2024-05-01T08:00:24Z

Python/import.c

            return NULL;
        }
-        assert(!PyErr_Occurred());
+        assert(!PyErr_Occurred() && res.err == NULL);


Let this tell you which one went wrong.

Suggested change

assert(!PyErr_Occurred() && res.err == NULL);

assert(!PyErr_Occurred());

assert(res.err == NULL);

encukou · 2024-05-01T08:07:56Z

Python/import.c

+
        // XXX __file__ doesn't get set!


This is now solved, file gets set elsewhere.

Suggested change

// XXX __file__ doesn't get set!

While the import machinery (in importlib._bootstrap) does set __file__, the only place it gets set for a single-phase init module via _imp.load_dynamic (ergo ExtensionFileLoader.exec_module()) is the first time it is loaded, in import_run_extension(). In the above case, the module was found in the global cache and gets reloaded from the cached init function.

I was going to clarify the comment, but it would be just as easy to actually set __file__ and be done with it. 😄

encukou · 2024-05-01T08:10:45Z

Python/import.c

        goto finally;
    }
-    assert(!PyErr_Occurred());
+    assert(!PyErr_Occurred() && res.err == NULL);


Suggested change

assert(!PyErr_Occurred() && res.err == NULL);

assert(!PyErr_Occurred());

assert(res.err == NULL);

encukou · 2024-05-01T08:37:42Z

Python/importdl.c

+    }
+
+    if (err->exc != NULL) {
+        PyErr_SetRaisedException(err->exc);


Consider warning the reader about that function's quirk:

Suggested change

PyErr_SetRaisedException(err->exc);

PyErr_SetRaisedException(err->exc);

err->exc = NULL; /* SetRaisedException stole our reference */

Maybe it would be better to merge _Py_ext_module_loader_result_apply_error and _Py_ext_module_loader_result_clear? They're effectively always used together (with apply skipped on success).

encukou · 2024-05-01T08:50:00Z

Python/importdl.c

            goto error;
        }
    }

-    assert(!PyErr_Occurred());
+    assert(!PyErr_Occurred() && res.err == NULL);


Suggested change

assert(!PyErr_Occurred() && res.err == NULL);

assert(!PyErr_Occurred());

aeesrt(res.err == NULL);

…sult_apply_error().

…ort Machinery (pythongh-118205) This change will make some later changes simpler.

ericsnowcurrently added the skip news label Apr 24, 2024

bedevere-app bot mentioned this pull request Apr 24, 2024

Single-Phase Init Extension Module Init Functions Still Run in Isolated Interpreters #117953

Open

ericsnowcurrently force-pushed the extension-kinds branch 2 times, most recently from 33af2db to 3724e9e Compare April 24, 2024 21:42

ericsnowcurrently mentioned this pull request Apr 26, 2024

gh-117953: Let update_global_state_for_extension() Caller Decide If Singlephase or Not #118193

Merged

ericsnowcurrently added 14 commits April 29, 2024 12:53

Add _Py_ext_module_loader_result.singlephase.

Unverified

The signing certificate or its chain could not be verified.

Certificate subject

CN phausler@apple.com

O Apple Inc.

Certificate issuer

CN Apple Corporate Signing ECC CA 1

O Apple Inc.

Learn about vigilant mode

79f39a4

res.singlephase -> res.kind

Unverified

The signing certificate or its chain could not be verified.

Certificate subject

CN phausler@apple.com

O Apple Inc.

Certificate issuer

CN Apple Corporate Signing ECC CA 1

O Apple Inc.

Learn about vigilant mode

aa78a49

Add _Py_ext_module_loader_result.err.

Unverified

The signing certificate or its chain could not be verified.

Certificate subject

CN phausler@apple.com

O Apple Inc.

Certificate issuer

CN Apple Corporate Signing ECC CA 1

O Apple Inc.

Learn about vigilant mode

938e50f

fix error case

Unverified

The signing certificate or its chain could not be verified.

Certificate subject

CN phausler@apple.com

O Apple Inc.

Certificate issuer

CN Apple Corporate Signing ECC CA 1

O Apple Inc.

Learn about vigilant mode

9383ce1

Add some asserts to _PyImport_RunModInitFunc().

Unverified

The signing certificate or its chain could not be verified.

Certificate subject

CN phausler@apple.com

O Apple Inc.

Certificate issuer

CN Apple Corporate Signing ECC CA 1

O Apple Inc.

Learn about vigilant mode

62166ae

Relax the error check.

9010c2c

Use an enum instead of storing an error string.

b08c5f5

_Py_ext_module_loader_result_kind -> _Py_ext_module_kind

8efc29f

Fix is_singlephase().

0359bae

Return UNKNOWN from get_extension_kind().

87e6bbc

Add a comment.

8622c28

Set m_init in update_global_state_for_extension().

7c7e654

Expand the cases in get_extension_kind().

4406192

ericsnowcurrently force-pushed the extension-kinds branch from 3724e9e to 4406192 Compare April 29, 2024 18:54

ericsnowcurrently marked this pull request as ready for review April 29, 2024 18:54

ericsnowcurrently requested review from kumaraditya303, brettcannon, ncoghlan and warsaw as code owners April 29, 2024 18:54

bedevere-app bot added the awaiting core review label Apr 29, 2024

ericsnowcurrently removed request for warsaw, ncoghlan, kumaraditya303 and brettcannon April 29, 2024 18:54

Add check_multiphase_preinit().

ae3e770

ericsnowcurrently requested a review from encukou April 29, 2024 19:39

encukou reviewed May 1, 2024

View reviewed changes

ericsnowcurrently added 8 commits May 1, 2024 13:43

Add a _Py_ext_module_kind typedef.

e94a082

check_singlephase() -> assert_singlephase()

4e1f308

Split up an assert.

aa1edc9

Set __file__ in the reload case.

cd365ec

Split up other asserts.

da1f91f

Clear the stolen exception.

c2a687c

Call _Py_ext_module_loader_result_clear() in _Py_ext_module_loader_re…

211dab3

…sult_apply_error().

Clarify the comment about modules sharing a def.

fca9857

ericsnowcurrently merged commit 526ca4c into python:main May 1, 2024
34 checks passed

bedevere-app bot removed the awaiting core review label May 1, 2024

ericsnowcurrently deleted the extension-kinds branch May 1, 2024 23:40

SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024

pythongh-117953: Work Relative to Specific Extension Kinds in the Imp…

39094af

…ort Machinery (pythongh-118205) This change will make some later changes simpler.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-117953: Work Relative to Specific Extension Kinds in the Import Machinery #118205

gh-117953: Work Relative to Specific Extension Kinds in the Import Machinery #118205

ericsnowcurrently commented Apr 24, 2024 •

edited by bedevere-app bot

Loading

Certificate subject

Certificate issuer

Certificate subject

Certificate issuer

Certificate subject

Certificate issuer

Certificate subject

Certificate issuer

Certificate subject

Certificate issuer

Certificate subject

Certificate issuer

ericsnowcurrently commented Apr 29, 2024

encukou left a comment

encukou May 1, 2024

ericsnowcurrently May 1, 2024

encukou May 1, 2024

ericsnowcurrently May 1, 2024

ericsnowcurrently May 1, 2024

encukou May 1, 2024

ericsnowcurrently May 1, 2024

encukou May 1, 2024

ericsnowcurrently May 1, 2024

ericsnowcurrently May 1, 2024

encukou May 1, 2024

ericsnowcurrently May 1, 2024

encukou May 1, 2024

ericsnowcurrently May 1, 2024

encukou May 1, 2024

ericsnowcurrently May 1, 2024

-            // XXX Two modules should not share the same def->m_base.
-            //assert(def->m_base.m_init == NULL
-            //           || def->m_base.m_init == singlephase->m_init);
+            // The following assert used to be skipped if
+            //     def->m_base.m_init == singlephase->m_init
+            // but two modules should not share the same def->m_base.
+            assert(def->m_base.m_init == NULL);

	assert(!PyErr_Occurred() && res.err == NULL);
	assert(!PyErr_Occurred());
	assert(res.err == NULL);

	PyErr_SetRaisedException(err->exc);
	PyErr_SetRaisedException(err->exc);
	err->exc = NULL; /* SetRaisedException stole our reference */

	assert(!PyErr_Occurred() && res.err == NULL);
	assert(!PyErr_Occurred());
	aeesrt(res.err == NULL);

gh-117953: Work Relative to Specific Extension Kinds in the Import Machinery #118205

gh-117953: Work Relative to Specific Extension Kinds in the Import Machinery #118205

Conversation

ericsnowcurrently commented Apr 24, 2024 • edited by bedevere-app bot Loading

ericsnowcurrently commented Apr 29, 2024

encukou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericsnowcurrently commented Apr 24, 2024 •

edited by bedevere-app bot

Loading