GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

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

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented Jul 21, 2023 •
edited by bedevere-bot
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?

We solve the problem by eliminating the one object we were still sharing between interpreters. We replace it with a low-level hashtable, using the "raw" allocator to avoid tying it to the main interpreter.

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

(This approach does not have the same potential performance penalty as the linked list we used in gh-106899.)

  • 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 4 commits July 21, 2023 15:15
@ericsnowcurrently
Add a comment.
3397777
@ericsnowcurrently
Add a TODO.
670e632
@ericsnowcurrently
Use a low-level hashtable instead of a dict.
485d130
@ericsnowcurrently
Drop _PyThreadState_*Detached().
77d5e8f
@ericsnowcurrently ericsnowcurrently added skip news needs backport to 3.12 only security fixes labels Jul 21, 2023
@ericsnowcurrently
ericsnowcurrently requested a review from Yhg1s July 21, 2023 21:19
@ericsnowcurrently
ericsnowcurrently requested review from brettcannon, ncoghlan and warsaw as code owners July 21, 2023 21:19
@bedevere-bot bedevere-bot added the awaiting core review label Jul 21, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jul 21, 2023
Crash During Subinterpreter Finalization #105699
Closed
@ericsnowcurrently
Set "byname" properly for the reload case.
04a19c4
@ericsnowcurrently

ericsnowcurrently commented Jul 25, 2023

Copy link
Copy Markdown
Member Author

@Yhg1s, any objections to this change in 3.12 (pending a review here)? I suspect the current use of dict will eventually cause a crash somewhere and more-so as folks use subinterpreters more. I'm asking here because I'd prefer 3.12 to match 3.13.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Yhg1s
Yhg1s reviewed Jul 27, 2023
View reviewed changes

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

Given that this fixes a fundamental issue, I'm okay with backporting it to 3.12rc1, if it can get into a decent shape in time.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Yhg1s
Yhg1s reviewed Jul 27, 2023
View reviewed changes
Comment thread Python/import.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
ericsnowcurrently added 3 commits July 27, 2023 11:50
@ericsnowcurrently
Use a flat hashtable instead of a nested one.
5fde127
@ericsnowcurrently
Merge branch 'main' into fix-extensions-tstate-alt
818d828
@ericsnowcurrently
Use the right allocator.
e83b0cc
Yhg1s
Yhg1s reviewed Jul 28, 2023
View reviewed changes
Comment thread Python/import.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/import.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/import.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/import.c Outdated

PyObject *extensions = EXTENSIONS.dict;
if (extensions == NULL) {
key = hashtable_key_from_2_strings(filename, name, ':');

@Yhg1s Yhg1s Jul 28, 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.

: is valid in filenames (on most filesystems). Why not /, which isn't? Also, since it's used in two places and not easy to miss/misinterpret, it should probably be a #define/static constant.

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Jul 28, 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.

An module name (identifier) will never have ":" or "/" in it. I figured the separator didn't matter as long as it wasn't a valid character in identifiers. I thought "/" might be a little confusing when debugging since it will likely be in the filename, so I picked the next character that made sense to me in this context, ":".

If you think there's an advantage to "/" (or any other character), or a disadvantage to ":" or you feel strongly about it, I'd be glad to change the separator. It isn't critical to me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/import.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/import.c Outdated
goto finally;
}
res = 0;
/* This decref would be problematic if the module def were

@Yhg1s Yhg1s Jul 28, 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.

That brings up a good point: shouldn't the PyModuleDefs placed in the shared cache be made immortal? Otherwise how can they sensibly be shared at all?

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Jul 28, 2023 •
edited
Loading

Uh oh!

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

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.

That makes sense. We don't use module defs like the usual objects and generally assume they are statically defined, but it would be good to play it safe.

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Jul 28, 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.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ericsnowcurrently and others added 6 commits July 28, 2023 10:01
@ericsnowcurrently @Yhg1s
Use PyUnicode_AsUTF8AndSize() (and fix the assert).
9e81cd5 
Co-authored-by: T. Wouters <[email protected]>
@ericsnowcurrently
Raise MemoryError where appropriate.
430716b
@ericsnowcurrently
Add a comment.
647fbc2
@ericsnowcurrently
Use a constant for the cache separator character.
bbe5eda
@ericsnowcurrently
Decref when clearing the hashtable.
1d7cbd0
@ericsnowcurrently
Make the cached module defs immortal.
20c8b9d
@ericsnowcurrently
ericsnowcurrently requested a review from Yhg1s July 28, 2023 16:38
Yhg1s
Yhg1s approved these changes Jul 28, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Jul 28, 2023
@ericsnowcurrently
ericsnowcurrently merged commit 8ba4df9 into python:main Jul 28, 2023
@miss-islington

miss-islington commented Jul 28, 2023

Copy link
Copy Markdown
Contributor

Thanks @ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Jul 28, 2023
@miss-islington

miss-islington commented Jul 28, 2023

Copy link
Copy Markdown
Contributor

Sorry, @ericsnowcurrently, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 8ba4df91ae60833723d8d3b9afeb2b642f7176d5 3.12
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently
ericsnowcurrently deleted the fix-extensions-tstate-alt branch July 28, 2023 20:39
@miss-islington miss-islington assigned ericsnowcurrently Jul 28, 2023
ericsnowcurrently added a commit to ericsnowcurrently/cpython that referenced this pull request Jul 28, 2023
@ericsnowcurrently
pythongh-105699: Use a _Py_hashtable_t for the PyModuleDef Cache (pyt…
5b3abd0 
…hongh-106974)

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?

We solve the problem by eliminating the one object we were still sharing between interpreters.  We replace it with a low-level hashtable, using the "raw" allocator to avoid tying it to the main interpreter.

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

(cherry picked from commit 8ba4df9)
@bedevere-bot

bedevere-bot commented Jul 28, 2023

Copy link
Copy Markdown

GH-107412 is a backport of this pull request to the 3.12 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Jul 28, 2023
@ericsnowcurrently

ericsnowcurrently commented Jul 28, 2023

Copy link
Copy Markdown
Member Author

Thanks for the reviews, @Yhg1s!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ericsnowcurrently added a commit that referenced this pull request Jul 28, 2023
@ericsnowcurrently
[3.12] gh-105699: Use a _Py_hashtable_t for the PyModuleDef Cache (gh…
da151fd 
…-106974) (gh-107412)

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

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?

We solve the problem by eliminating the one object we were still sharing between interpreters.  We replace it with a low-level hashtable, using the "raw" allocator to avoid tying it to the main interpreter.

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

(cherry picked from commit 8ba4df9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Yhg1s Yhg1s Yhg1s approved these changes
@brettcannon brettcannon Awaiting requested review from brettcannon brettcannon is a code owner
@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner
@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner

Uh oh!

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

Assignees

@ericsnowcurrently ericsnowcurrently

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.

4 participants

@ericsnowcurrently @miss-islington @bedevere-bot @Yhg1s
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.