Skip to content

bpo-44032: Defer clearing last thread state until last GC has been run. #26285

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
merged 2 commits into from
May 24, 2021
Merged

bpo-44032: Defer clearing last thread state until last GC has been run. #26285

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
236beca
Defer clearing last thread state until last GC has been run.
markshannon May 21, 2021
ddad17e
Delay deletion of stack chunks until thread state is deleted.
markshannon May 22, 2021
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
14 changes: 7 additions & 7 deletions Python/pystate.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -897,13 +897,6 @@ PyThreadState_Clear(PyThreadState *tstate)
if (tstate->on_delete != NULL) {
tstate->on_delete(tstate->on_delete_data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also be moved to tstate_delete_common, or is it unrelated to deleting the data stack?

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 have no idea what it does, so I'm not touching it 🙂

Copy link
Contributor

@erlend-aasland erlend-aasland May 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Me neither :) I tried moving it, and it seems to work fine either place. BTW, I found a clue in Include:

cpython/Include/cpython/pystate.h

Lines 134 to 143 in a9e4361

* So instead of holding the lock directly, the tstate holds a weakref to
* the lock: that's the value of on_delete_data below. Decref'ing a
* weakref is harmless.
* on_delete points to _threadmodule.c's static release_sentinel() function.
* After the tstate is unlinked, release_sentinel is called with the
* weakref-to-lock (on_delete_data) argument, and release_sentinel releases
* the indirectly held lock.
*/
void (*on_delete)(void *);
void *on_delete_data;

Copy link
Contributor

Choose a reason for hiding this comment

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

Also: https://docs.python.org/3/c-api/init.html#c.PyThreadState_Clear:

"Changed in version 3.9: This function now calls the PyThreadState.on_delete callback. Previously, that happened in PyThreadState_Delete()."

Copy link
Contributor

@erlend-aasland erlend-aasland May 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

... and here:

cpython/Modules/_threadmodule.c

Lines 1280 to 1329 in a9e4361

release_sentinel(void *wr_raw)
{
PyObject *wr = _PyObject_CAST(wr_raw);
/* Tricky: this function is called when the current thread state
is being deleted. Therefore, only simple C code can safely
execute here. */
PyObject *obj = PyWeakref_GET_OBJECT(wr);
lockobject *lock;
if (obj != Py_None) {
lock = (lockobject *) obj;
if (lock->locked) {
PyThread_release_lock(lock->lock_lock);
lock->locked = 0;
}
}
/* Deallocating a weakref with a NULL callback only calls
PyObject_GC_Del(), which can't call any Python code. */
Py_DECREF(wr);
}
static PyObject *
thread__set_sentinel(PyObject *module, PyObject *Py_UNUSED(ignored))
{
PyObject *wr;
PyThreadState *tstate = PyThreadState_Get();
lockobject *lock;
if (tstate->on_delete_data != NULL) {
/* We must support the re-creation of the lock from a
fork()ed child. */
assert(tstate->on_delete == &release_sentinel);
wr = (PyObject *) tstate->on_delete_data;
tstate->on_delete = NULL;
tstate->on_delete_data = NULL;
Py_DECREF(wr);
}
lock = newlockobject(module);
if (lock == NULL)
return NULL;
/* The lock is owned by whoever called _set_sentinel(), but the weakref
hangs to the thread state. */
wr = PyWeakref_NewRef((PyObject *) lock, NULL);
if (wr == NULL) {
Py_DECREF(lock);
return NULL;
}
tstate->on_delete_data = (void *) wr;
tstate->on_delete = &release_sentinel;
return (PyObject *) lock;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It's added by @vstinner in GH-18296 (issue 39511). I guess he knows if it should stay or follow, then :)

}
_PyStackChunk *chunk = tstate->datastack_chunk;
tstate->datastack_chunk = NULL;
while (chunk != NULL) {
_PyStackChunk *prev = chunk->previous;
_PyObject_VirtualFree(chunk, chunk->size);
chunk = prev;
}
}


Expand Down Expand Up @@ -936,6 +929,13 @@ tstate_delete_common(PyThreadState *tstate,
{
PyThread_tss_set(&gilstate->autoTSSkey, NULL);
}
_PyStackChunk *chunk = tstate->datastack_chunk;
tstate->datastack_chunk = NULL;
while (chunk != NULL) {
_PyStackChunk *prev = chunk->previous;
_PyObject_VirtualFree(chunk, chunk->size);
chunk = prev;
}
}

static void
Expand Down