Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.12] gh-119213: Fix getargs.c to store state in InterpreterState... #119194

Closed
wants to merge 4 commits into from

Conversation

1st1
Copy link
Member

@1st1 1st1 commented May 20, 2024

...as opposed to storing it in PyRuntime. Storing it in PyRuntime is fundametally wrong, as its state contains references to Python objects. Those objects (tuples and strings) can (and will) be picked by various subinterpreter clean up code, leaving PyRuntime with broken pointers.

...as opposed to storing it in PyRuntime. Storing it in PyRuntime
is fundametally wrong, as its state contains references to Python
objects. Those objects (tuples and strings) can (and will) be
picked by various subinterpreter clean up code, leaving PyRuntime
with broken pointers.
@@ -27,11 +27,6 @@ extern "C" {
#include "pycore_typeobject.h" // struct types_runtime_state
#include "pycore_unicodeobject.h" // struct _Py_unicode_runtime_ids

struct _getargs_runtime_state {
PyThread_type_lock mutex;
Copy link
Member Author

Choose a reason for hiding this comment

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

I think having a lock isn't necessary in 3.12 as we're muving this struct to InterpreterState. Local state modification will be protected by the GIL.

@ericsnowcurrently ericsnowcurrently changed the title Fix getargs.c to store state in InterpreterState... gh-119213: Fix getargs.c to store state in InterpreterState... May 20, 2024
@Eclips4 Eclips4 changed the title gh-119213: Fix getargs.c to store state in InterpreterState... [3.12] gh-119213: Fix getargs.c to store state in InterpreterState... May 20, 2024
@ericsnowcurrently
Copy link
Member

superseded by gh-119425

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants