gh-117953: Work Relative to Specific Extension Kinds in the Import Machinery by ericsnowcurrently · Pull Request #118205 · python/cpython · GitHub

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.

@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).

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

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.

done

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.

done

Let this tell you which one went wrong.

Suggested change

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

done

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. 😄

done

Suggested change

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

done

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).

done

Suggested change

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

done

…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.

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

	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 by ericsnowcurrently · Pull Request #118205 · python/cpython · GitHub

Uh oh!

Uh oh!

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#118205ericsnowcurrently merged 23 commits intopython:mainpython/cpython:mainfrom ericsnowcurrently:extension-kindsericsnowcurrently/cpython:extension-kindsCopy head branch name to clipboard

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

gh-117953: Work Relative to Specific Extension Kinds in the Import Machinery#118205
ericsnowcurrently merged 23 commits into
python:mainfrom
ericsnowcurrently:extension-kinds

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