GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-117953: Work Relative to Specific Extension Kinds in the Import Machinery by ericsnowcurrently · Pull Request #118205 · python/cpython

Conversation

ericsnowcurrently commented Apr 24, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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…

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

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

ericsnowcurrently commented Apr 29, 2024

Copy link

Copy Markdown

Member Author

@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 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

encukou reviewed

May 1, 2024

View reviewed changes

encukou left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/import.c Outdated

		}

		static bool
		check_multiphase_preinit(PyModuleDef *def)

encukou May 1, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently reacted with thumbs up emoji

All reactions

👍 1 reaction

ericsnowcurrently May 1, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/import.c Outdated

Comment on lines +1264 to +1266

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

encukou May 1, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently May 1, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently May 1, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/import.c Outdated

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

encukou May 1, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let this tell you which one went wrong.

Suggested change

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently reacted with thumbs up emoji

All reactions

👍 1 reaction

ericsnowcurrently May 1, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/import.c Outdated

Comment on lines 1426 to 1427


		// XXX __file__ doesn't get set!

encukou May 1, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now solved, file gets set elsewhere.

Suggested change


	// XXX __file__ doesn't get set!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently May 1, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

encukou reacted with thumbs up emoji

All reactions

👍 1 reaction

ericsnowcurrently May 1, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/import.c Outdated

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

encukou May 1, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently May 1, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/importdl.c Outdated

		}

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

encukou May 1, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently reacted with thumbs up emoji

All reactions

👍 1 reaction

ericsnowcurrently May 1, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/importdl.c Outdated

		}

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

encukou May 1, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently May 1, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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.

Reviewers

encukou

encukou left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ericsnowcurrently commented Apr 24, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.