FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-39877: take_gil() now exits the thread if finalizing by vstinner · Pull Request #18854 · python/cpython · GitHub

/ cpython Public

bpo-39877: take_gil() now exits the thread if finalizing #18854

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

Closed
vstinner wants to merge 2 commits into from
Conversation 12 Commits 2 Checks 0 Files changed
Closed

bpo-39877: take_gil() now exits the thread if finalizing #18854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ac0b25
bpo-39877: take_gil() now exits the thread if finalizing
vstinner Mar 8, 2020
4f379aa
take_gil() exit after COND_TIMED_WAIT()
vstinner Mar 8, 2020
File filter

Filter by extension

Filter by extension .c  (1) .h  (1) All 2 file types selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
66 changes: 7 additions & 59 deletions Python/ceval.c
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -197,16 +197,6 @@ ensure_tstate_not_null(const char *func, PyThreadState *tstate)
}


#ifndef NDEBUG
static int is_tstate_valid(PyThreadState *tstate)
{
assert(!_PyMem_IsPtrFreed(tstate));
assert(!_PyMem_IsPtrFreed(tstate->interp));
return 1;
}
#endif


int
PyEval_ThreadsInitialized(void)
{
Expand All @@ -228,7 +218,8 @@ PyEval_InitThreads(void)
create_gil(gil);
PyThreadState *tstate = _PyRuntimeState_GetThreadState(runtime);
ensure_tstate_not_null(__func__, tstate);
take_gil(ceval, tstate);

take_gil(tstate);

struct _pending_calls *pending = &ceval->pending;
pending->lock = PyThread_allocate_lock();
Expand All @@ -255,29 +246,6 @@ _PyEval_FiniThreads(struct _ceval_runtime_state *ceval)
}
}

/* This function is designed to exit daemon threads immediately rather than
taking the GIL if Py_Finalize() has been called.

The caller must *not* hold the GIL, since this function does not release
the GIL before exiting the thread.

When this function is called by a daemon thread after Py_Finalize() has been
called, the GIL does no longer exist.

tstate must be non-NULL. */
static inline void
exit_thread_if_finalizing(PyThreadState *tstate)
{
/* bpo-39877: Access _PyRuntime directly rather than using
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. */
_PyRuntimeState *runtime = &_PyRuntime;
PyThreadState *finalizing = _PyRuntimeState_GetFinalizing(runtime);
if (finalizing != NULL && finalizing != tstate) {
PyThread_exit_thread();
}
}

void
_PyEval_Fini(void)
Expand Down Expand Up @@ -315,11 +283,8 @@ PyEval_AcquireLock(void)
PyThreadState *tstate = _PyRuntimeState_GetThreadState(runtime);
ensure_tstate_not_null(__func__, tstate);

exit_thread_if_finalizing(tstate);
assert(is_tstate_valid(tstate));
take_gil(tstate);

struct _ceval_runtime_state *ceval = &runtime->ceval;
take_gil(ceval, tstate);
}

void
Expand All @@ -339,16 +304,9 @@ PyEval_AcquireThread(PyThreadState *tstate)
{
ensure_tstate_not_null(__func__, tstate);

exit_thread_if_finalizing(tstate);
assert(is_tstate_valid(tstate));
take_gil(tstate);

_PyRuntimeState *runtime = tstate->interp->runtime;
struct _ceval_runtime_state *ceval = &runtime->ceval;

/* Check someone has called PyEval_InitThreads() to create the lock */
assert(gil_created(&ceval->gil));

take_gil(ceval, tstate);
if (_PyThreadState_Swap(&runtime->gilstate, tstate) != NULL) {
Py_FatalError("non-NULL old thread state");
}
Expand Down Expand Up @@ -382,7 +340,7 @@ _PyEval_ReInitThreads(_PyRuntimeState *runtime)
recreate_gil(&ceval->gil);
PyThreadState *tstate = _PyRuntimeState_GetThreadState(runtime);
ensure_tstate_not_null(__func__, tstate);
take_gil(ceval, tstate);
take_gil(tstate);

struct _pending_calls *pending = &ceval->pending;
pending->lock = PyThread_allocate_lock();
Expand Down Expand Up @@ -422,15 +380,9 @@ PyEval_RestoreThread(PyThreadState *tstate)
{
ensure_tstate_not_null(__func__, tstate);

exit_thread_if_finalizing(tstate);
assert(is_tstate_valid(tstate));
take_gil(tstate);

_PyRuntimeState *runtime = tstate->interp->runtime;
struct _ceval_runtime_state *ceval = &runtime->ceval;
assert(gil_created(&ceval->gil));

take_gil(ceval, tstate);

_PyThreadState_Swap(&runtime->gilstate, tstate);
}

Expand Down Expand Up @@ -793,7 +745,6 @@ _PyEval_EvalFrameDefault(PyFrameObject *f, int throwflag)

PyThreadState * const tstate = _PyRuntimeState_GetThreadState(runtime);
ensure_tstate_not_null(__func__, tstate);
assert(is_tstate_valid(tstate));

/* when tracing we set things up so that

Expand Down Expand Up @@ -1282,10 +1233,7 @@ _PyEval_EvalFrameDefault(PyFrameObject *f, int throwflag)

/* Other threads may run now */

/* Check if we should make a quick exit. */
exit_thread_if_finalizing(tstate);

take_gil(ceval, tstate);
take_gil(tstate);

if (_PyThreadState_Swap(&runtime->gilstate, tstate) != NULL) {
Py_FatalError("orphan tstate");
Expand Down
58 changes: 57 additions & 1 deletion Python/ceval_gil.h
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -180,17 +180,57 @@ drop_gil(struct _ceval_runtime_state *ceval, PyThreadState *tstate)
#endif
}


static inline int
thread_must_exit(PyThreadState *tstate)
{
/* bpo-39877: Access _PyRuntime directly rather than using
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. */
_PyRuntimeState *runtime = &_PyRuntime;
PyThreadState *finalizing = _PyRuntimeState_GetFinalizing(runtime);
return (finalizing != NULL && finalizing != tstate);
}


/* Take the GIL.

The function saves errno at entry and restores its value at exit.

Exit immediately the thread if Py_Finalize() has been called and tstate is
not the thread which called Py_Finalize(). For example, exit daemon threads
which survive after Py_Finalize().

tstate must be non-NULL. */
static void
take_gil(struct _ceval_runtime_state *ceval, PyThreadState *tstate)
take_gil(PyThreadState *tstate)
{
int err = errno;

if (thread_must_exit(tstate)) {

pitrou Mar 8, 2020

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ok, but shouldn't you do this after COND_TIMED_WAIT below as well?
/* bpo-39877: If Py_Finalize() has been called and tstate is not the
thread which called Py_Finalize(), exit immediately the thread.

This code path can be reached by a daemon thread after Py_Finalize()
completes. In this case, tstate is a dangling pointer: points to
PyThreadState freed memory.

When this function is called after Py_Finalize() completed, the GIL
does no longer exist. */
PyThread_exit_thread();
}

/* ensure that tstate is valid */
assert(!_PyMem_IsPtrFreed(tstate));
assert(!_PyMem_IsPtrFreed(tstate->interp));

struct _ceval_runtime_state *ceval = &tstate->interp->runtime->ceval;
struct _gil_runtime_state *gil = &ceval->gil;

/* Check someone has called PyEval_InitThreads() to create the lock */
assert(gil_created(gil));

MUTEX_LOCK(gil->mutex);

if (!_Py_atomic_load_relaxed(&gil->locked)) {
Expand All @@ -206,6 +246,20 @@ take_gil(struct _ceval_runtime_state *ceval, PyThreadState *tstate)

unsigned long interval = (gil->interval >= 1 ? gil->interval : 1);
COND_TIMED_WAIT(gil->cond, gil->mutex, interval, timed_out);

if (thread_must_exit(tstate)) {
/* bpo-36475: If Py_Finalize() has been called and tstate is not
the thread which called Py_Finalize(), exit immediately the
thread.

This code path can be reached by a daemon thread which continues
to run after wait_for_thread_shutdown() and before Py_Finalize()
completes. For example, when _PyImport_Cleanup() executes Python
code. */
MUTEX_UNLOCK(gil->mutex);

vstinner Mar 8, 2020

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm not sure if anyone else should be done here.

vstinner Mar 9, 2020

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

For example, is COND_SIGNAL(gil->cond) needed to wake up the next thread waiting on COND_TIMED_WAIT()?
PyThread_exit_thread();
}

/* If we timed out and no switch occurred in the meantime, it is time
to ask the GIL-holding thread to drop it. */
if (timed_out &&
Expand Down Expand Up @@ -243,6 +297,8 @@ take_gil(struct _ceval_runtime_state *ceval, PyThreadState *tstate)

MUTEX_UNLOCK(gil->mutex);

assert(!thread_must_exit(tstate));

errno = err;
}

Expand Down
Back | FazBrowse Home | New Git URL