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

    }

-    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

Closed

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.

79f39a4

res.singlephase -> res.kind

aa78a49

Add _Py_ext_module_loader_result.err.

938e50f

fix error case

9383ce1

Add some asserts to _PyImport_RunModInitFunc().

62166ae

Check the module returned by import_find_extension() to ensure single…

4881119

…-phase init.

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 brettcannon, kumaraditya303, 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 brettcannon, kumaraditya303, ncoghlan and warsaw 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

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.

-            // 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);

Uh oh!

Conversation

ericsnowcurrently commented Apr 24, 2024 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently commented Apr 29, 2024

Uh oh!

encukou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ericsnowcurrently commented Apr 24, 2024 •

edited by bedevere-app bot

Loading