Skip to content

gh-109793: Allow Switching Interpreters During Finalization #109794

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.

Sign up for GitHub

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

Merged
ericsnowcurrently merged 9 commits into from
Sep 27, 2023
Merged

gh-109793: Allow Switching Interpreters During Finalization #109794

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5e2224f
Add a test.
ericsnowcurrently Sep 23, 2023
a1014a9
Also check the thread ID in _PyThreadState_MustExit().
ericsnowcurrently Sep 23, 2023
d46103d
Use _PyRuntime directly in _Py_IsMainInterpreterFinalizing().
ericsnowcurrently Sep 23, 2023
2e13a75
Add _PyRuntimeState._finalizing_id and _PyInterpreterState._finalizin…
ericsnowcurrently Sep 24, 2023
756cc3b
Add verbose output to the test.
ericsnowcurrently Sep 25, 2023
ad312b9
Add a NEWS entry.
ericsnowcurrently Sep 25, 2023
8feeb4c
Skip the assert for now.
ericsnowcurrently Sep 25, 2023
9e589a7
Use the new _Py_atomic API.
ericsnowcurrently Sep 27, 2023
9829315
Fix the NEWS entry.
ericsnowcurrently Sep 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add _PyRuntimeState._finalizing_id and _PyInterpreterState._finalizin… 
…g_id.
  • Loading branch information
@ericsnowcurrently
ericsnowcurrently committed Sep 25, 2023
commit 2e13a75928057758f650ad22eaeb7d56ec728120
24 changes: 24 additions & 0 deletions Include/internal/pycore_atomic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ typedef struct _Py_atomic_address {
atomic_uintptr_t _value;
} _Py_atomic_address;

typedef struct _Py_atomic_ulong {

@markshannon markshannon Sep 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you use either _Py_atomic_load_uint32 or _Py_atomic_load_uint64 instead of defining your own.
Since thread id is an unsigned long, you'll need to use a macro, but it seems better to use the pre-existing functions.

#if SIZEOF_LONG == 8
#define _Py_atomic_load_ulong _Py_atomic_load_uint64
#elif SIZEOF_LONG == 4
#define _Py_atomic_load_ulong _Py_atomic_load_uint32
#else
#error "long must be 4 or 8 bytes in size"
#endif

@ericsnowcurrently ericsnowcurrently Sep 27, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

atomic_ulong _value;
} _Py_atomic_ulong;

typedef struct _Py_atomic_int {
atomic_int _value;
} _Py_atomic_int;
Expand Down Expand Up @@ -77,6 +81,10 @@ typedef struct _Py_atomic_address {
uintptr_t _value;
} _Py_atomic_address;

typedef struct _Py_atomic_ulong {
unsigned long _value;
} _Py_atomic_ulong;

typedef struct _Py_atomic_int {
int _value;
} _Py_atomic_int;
Expand Down Expand Up @@ -115,6 +123,10 @@ typedef struct _Py_atomic_address {
uintptr_t _value;
} _Py_atomic_address;

typedef struct _Py_atomic_ulong {
unsigned long _value;
} _Py_atomic_ulong;

typedef struct _Py_atomic_int {
int _value;
} _Py_atomic_int;
Expand Down Expand Up @@ -251,6 +263,10 @@ typedef struct _Py_atomic_address {
volatile uintptr_t _value;
} _Py_atomic_address;

typedef struct _Py_atomic_ulong {
volatile unsigned long _value;
} _Py_atomic_ulong;

typedef struct _Py_atomic_int {
volatile int _value;
} _Py_atomic_int;
Expand Down Expand Up @@ -387,6 +403,10 @@ typedef struct _Py_atomic_address {
volatile uintptr_t _value;
} _Py_atomic_address;

typedef struct _Py_atomic_ulong {
volatile unsigned long _value;
} _Py_atomic_ulong;

typedef struct _Py_atomic_int {
volatile int _value;
} _Py_atomic_int;
Expand Down Expand Up @@ -524,6 +544,10 @@ typedef struct _Py_atomic_address {
uintptr_t _value;
} _Py_atomic_address;

typedef struct _Py_atomic_ulong {
unsigned long _value;
} _Py_atomic_ulong;

typedef struct _Py_atomic_int {
int _value;
} _Py_atomic_int;
Expand Down
14 changes: 14 additions & 0 deletions Include/internal/pycore_interp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ struct _is {
and _PyInterpreterState_SetFinalizing()
to access it, don't access it directly. */
_Py_atomic_address _finalizing;
/* The ID of the OS thread in which we are finalizing. */
_Py_atomic_ulong _finalizing_id;

struct _gc_runtime_state gc;

Expand Down Expand Up @@ -215,9 +217,21 @@ _PyInterpreterState_GetFinalizing(PyInterpreterState *interp) {
return (PyThreadState*)_Py_atomic_load_relaxed(&interp->_finalizing);
}

static inline unsigned long
_PyInterpreterState_GetFinalizingID(PyInterpreterState *interp) {
return _Py_atomic_load_relaxed(&interp->_finalizing_id);
}

static inline void
_PyInterpreterState_SetFinalizing(PyInterpreterState *interp, PyThreadState *tstate) {
_Py_atomic_store_relaxed(&interp->_finalizing, (uintptr_t)tstate);
if (tstate == NULL) {
_Py_atomic_store_relaxed(&interp->_finalizing_id, 0);
}
else {
assert(tstate->thread_id == PyThread_get_thread_ident());
_Py_atomic_store_relaxed(&interp->_finalizing_id, tstate->thread_id);
}
}


Expand Down
14 changes: 14 additions & 0 deletions Include/internal/pycore_runtime.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,8 @@ typedef struct pyruntimestate {
Use _PyRuntimeState_GetFinalizing() and _PyRuntimeState_SetFinalizing()
to access it, don't access it directly. */
_Py_atomic_address _finalizing;
/* The ID of the OS thread in which we are finalizing. */
_Py_atomic_ulong _finalizing_id;

struct pyinterpreters {
PyThread_type_lock mutex;
Expand Down Expand Up @@ -303,9 +305,21 @@ _PyRuntimeState_GetFinalizing(_PyRuntimeState *runtime) {
return (PyThreadState*)_Py_atomic_load_relaxed(&runtime->_finalizing);
}

static inline unsigned long
_PyRuntimeState_GetFinalizingID(_PyRuntimeState *runtime) {
return _Py_atomic_load_relaxed(&runtime->_finalizing_id);
}

static inline void
_PyRuntimeState_SetFinalizing(_PyRuntimeState *runtime, PyThreadState *tstate) {
_Py_atomic_store_relaxed(&runtime->_finalizing, (uintptr_t)tstate);
if (tstate == NULL) {
_Py_atomic_store_relaxed(&runtime->_finalizing_id, 0);
}
else {
assert(tstate->thread_id == PyThread_get_thread_ident());
_Py_atomic_store_relaxed(&runtime->_finalizing_id, tstate->thread_id);
}
}

#ifdef __cplusplus
Expand Down
7 changes: 6 additions & 1 deletion Python/pystate.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2964,17 +2964,22 @@ _PyThreadState_MustExit(PyThreadState *tstate)
tstate->interp->runtime to support calls from Python daemon threads.
After Py_Finalize() has been called, tstate can be a dangling pointer:
point to PyThreadState freed memory. */
unsigned long finalizing_id = _PyRuntimeState_GetFinalizingID(&_PyRuntime);
PyThreadState *finalizing = _PyRuntimeState_GetFinalizing(&_PyRuntime);
if (finalizing == NULL) {
// XXX This isn't completely safe from daemon thraeds,
// since tstate might be a dangling pointer.
finalizing = _PyInterpreterState_GetFinalizing(tstate->interp);
finalizing_id = _PyInterpreterState_GetFinalizingID(tstate->interp);
}
// XXX else check &_PyRuntime._main_interpreter._initial_thread
if (finalizing == NULL) {
return 0;
}
else if (finalizing == tstate) {
return 0;
}
else if (finalizing->thread_id == tstate->thread_id) {
else if (finalizing_id == PyThread_get_thread_ident()) {
/* gh-109793: we must have switched interpreters. */
return 0;
}
Expand Down