GeezerVison

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

GeezerVison is showing github.com — gh-105699: Use a Linked List for the PyModuleDef Cache by ericsnowcurrently · Pull Request #106899 · python/cpython

Conversation

ericsnowcurrently commented Jul 19, 2023 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

This fixes a crasher due to a race condition, triggered infrequently when two isolated (own GIL) subinterpreters simultaneously initialize their sys or builtins modules. The crash happened due the combination of the "detached" thread state we were using and the "last holder" logic we use for the GIL. It turns out it's tricky to use the same thread state for different threads. Who could have guessed? <wink/>

We solve the problem by eliminating the one object we were still sharing between interpreters. We replace it with a linked list, using the "raw" allocator to avoid tying it to the main interpreter. We assume that there will be few enough legacy extension modules loaded that the O(n) operations won't cause too much overhead.

We also remove the accommodations for "detached" thread states, which were a dubious idea to start with.

Issue: Crash During Subinterpreter Finalization #105699

Sorry, something went wrong.

Uh oh!

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

All reactions

ericsnowcurrently added 5 commits

July 19, 2023 07:50


          Add a comment.

31414a3


          Add a TODO.

a6f6c0c


          Use a linked list instead of a dict object.

4c99023


          Drop _PyThreadState_*Detached().

d5b3aa4


          Add a NEWS entry.

fb5a9d3

ericsnowcurrently added the needs backport to 3.12 only security fixes label

Jul 19, 2023

ericsnowcurrently requested a review from Yhg1s

July 19, 2023 16:15

ericsnowcurrently requested review from brettcannon, ncoghlan and warsaw as code owners

July 19, 2023 16:15

bedevere-bot added the awaiting core review label

Jul 19, 2023

bedevere-bot mentioned this pull request

Jul 19, 2023

Crash During Subinterpreter Finalization #105699

Closed

Yhg1s reviewed

Jul 21, 2023

View reviewed changes

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

I'm not sure the assumption that a linked list is good enough because the number of extensions is small is actually warranted, and I think we should definitely double-check the performance on systems with non-trivial numbers of modules. Not using a dict for the cache avoids some problems, but replacing it with a linear search is very likely to have a bad performance hit.

Since the problem being fixed only manifests itself with isolated subinterpreters, I would rather live with the race in 3.12 than rush in a big performance regression, even if it's just for imports of extension modules.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/import.c

		_PyInterpreterState_Main());
		/* The lock is initialized directly with the general runtime state. */
		assert(EXTENSIONS.mutex != NULL);
		//assert(EXTENSIONS.head == NULL);

Yhg1s Jul 21, 2023

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.

Leftover line I presume.

Sorry, something went wrong.

Uh oh!

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

All reactions

ericsnowcurrently Jul 21, 2023

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.

Sort of. I had expected the assert to work (and it would in a simpler world). I was going to circle back to it but, having slept on it, don't see the need. It can be dropped.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/import.c

		@@ -919,167 +919,134 @@ extensions_lock_release(void)
		static void
		_extensions_cache_init(void)

Yhg1s Jul 21, 2023

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.

Why have this function at all, now that it's just a single assert?

Sorry, something went wrong.

Uh oh!

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

All reactions

ericsnowcurrently Jul 21, 2023

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.

Honestly, I figured I'd end up finding a better solution than a linked list that would revive the need for this function, so I kept it around. The point is a bit moot now though. 😄

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/import.c

		PyErr_NoMemory();
		goto finally;
		}
		strcpy(cached->filename, PyUnicode_AsUTF8(filename));

Yhg1s Jul 21, 2023

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.

Use strncpy to avoid the deprecation in some compilers (macOS IIRC?)

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

Comment thread

Python/import.c

		}
		res = 0;

		/* Destroy the cache data. */

Yhg1s Jul 21, 2023

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.

Shouldn't this DECREF found->def ?

Sorry, something went wrong.

Uh oh!

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

All reactions

ericsnowcurrently Jul 21, 2023

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.

good catch

Sorry, something went wrong.

Uh oh!

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

All reactions

ericsnowcurrently commented Jul 21, 2023

Copy link

Copy Markdown

Member Author

Understood. I was looking for a quick fix and mostly played a hunch that the performance impact would be minimal. I don't think I'm wrong but agree there isn't enough time to properly validate my hypothesis. Thanks for speaking up.

I do plan on finding an alternative that doesn't rely on a Python object, to address the crasher.

All reactions

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently closed this

Jul 21, 2023

ericsnowcurrently mentioned this pull request

Jul 21, 2023

gh-105699: Use a _Py_hashtable_t for the PyModuleDef Cache #106974

Merged

Reviewers

Yhg1s

Yhg1s left review comments

brettcannon

Awaiting requested review from brettcannon

brettcannon is a code owner

ncoghlan

Awaiting requested review from ncoghlan

ncoghlan is a code owner

warsaw

Awaiting requested review from warsaw

warsaw is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

awaiting core review needs backport to 3.12

only security fixes

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.

3 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 01402503 — 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 Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yhg1s 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!

ericsnowcurrently commented Jul 21, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ericsnowcurrently commented Jul 19, 2023 •

edited

Loading

Uh oh!

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