gh-104812: Run Pending Calls in any Thread #104813

ericsnowcurrently · 2023-05-23T19:51:37Z

For a while now, pending calls only run in the main thread (in the main interpreter). This PR changes things to allow any thread run a pending call, unless the pending call was explicitly added for the main thread to run.

(For reviewers: almost all of the significant change is in ceval_gil.c. The bulk of this PR's diff is comprised of tests and a long comment about the eval breaker.)

Issue: Subinterpreter Pending Calls Never Run #104812

…hreads().

bedevere-bot · 2023-06-02T21:58:11Z

🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit fdde46d 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

ericsnowcurrently · 2023-06-06T23:07:21Z

@markshannon, did you have a chance to look this over?

markshannon

This seems fine, but I'm not an expert on this, so another review would be appreciated

markshannon · 2023-06-13T12:47:27Z

Python/ceval.c

+     *  - signal handling (only in the main thread)
+     *
+     * When the need for one of the above is detected, the eval loop
+     * calls _Py_HandlePending() (from ceval_gil.c).  Then, if that


I wouldn't explain how it works in terms of specific functions and files, as the explanation will be out of date all too soon.

markshannon · 2023-06-13T12:50:22Z

Python/ceval.c

+     *
+     * One consequence of this approach is that it can be tricky
+     * to force any specific thread to pick up the eval breaker,
+     * or for any specific thread to not pick it up.


Is this true? The delay from setting the eval_breaker to the running thread responding to it should be in the order of a microsecond in the interpreter.

The problem is long running C extensions that don't check for interrupts. But that was a problem historically as well.

I wrote from experience. 😄 It wasn't obvious to me how to write tests that ran pending calls in specific threads. I'll elaborate.

markshannon · 2023-06-13T12:51:20Z

Python/ceval.c

+     * "handle_eval_breaker" label.  Some instructions come here
+     * explicitly (goto) and some indirectly via the CHECK_EVAL_BREAKER
+     * macro (see ceval_macros.h).  Notably, the check happens at the
+     * end of the JUMP_BACKWARD instruction, which pretty much applies


Say, "on back edges in the control flow graph", rather than mentioning the specific bytecode.

markshannon · 2023-06-13T12:52:23Z

Python/ceval_gil.c

-           && _Py_ThreadCanHandlePendingCalls())
+        | (_Py_atomic_load_relaxed_int32(&ceval2->pending.calls_to_do))
+        | (_Py_IsMainThread() && _Py_IsMainInterpreter(interp)
+           &&_Py_atomic_load_relaxed_int32(&ceval->pending_mainthread.calls_to_do))


COMPUTE_EVAL_BREAKER is getting unwieldy. Any thoughts on how to clean it up? (for another PR)

I don't have any recommendations at the moment, but agree it would be worth at least thinking of alternatives. That said, the current code is fairly straightforward (a mostly flat enumeration of conditions), which helps readers to follow it at a glance.

markshannon · 2023-06-13T12:57:04Z

Python/ceval_gil.c

-        interp = _PyInterpreterState_Main();
-    }
-    return _PyEval_AddPendingCall(interp, func, arg);
+    /* Legacy users of this API will continue to target the main thread. */


Maybe clarify?
This means the "the main thread of the main interpreter", not the "the main thread of the current interpreter"?

markshannon · 2023-06-13T13:06:32Z

Python/ceval_gil.c

@@ -95,11 +96,11 @@ RESET_GIL_DROP_REQUEST(PyInterpreterState *interp)


 static inline void
-SIGNAL_PENDING_CALLS(PyInterpreterState *interp)
+SIGNAL_PENDING_CALLS(struct _pending_calls *pending, PyInterpreterState *interp)


Want to make this and the unsignal version less SHOUTY, as it isn't a macro anymore?

Also, the name "SIGNAL" is a bit confusing as this doesn't handle signals, but code around here does.

The same applies to all the other static inline functions that used to be macros. I'd rather leave renaming them all to a separate change. I'm not sure the churn is worth it either.

markshannon · 2023-06-13T13:18:49Z

Misc/NEWS.d/next/Core and Builtins/2023-06-02-15-15-41.gh-issue-104812.dfZiG5.rst

+calls are still only done in the main thread, ergo in the main interpreter.
+This change does not affect the existing public C-API
+(``Py_AddPendingCall()``) which still only targets the main thread. The new
+functionality is meant strictly for internal use.  This change brings the


Why is it "meant strictly for internal use"? What bad things happen if a third party uses it?

Pending calls can run anything and cause state changes outside the normal flow of execution. Consequently, it needs to be used carefully because it isn't clear what the potential risks are. So until we take the time to figure all that out, we are keeping it strictly internal-only.

I've clarified the note.

ericsnowcurrently · 2023-06-13T16:34:26Z

Unless there are any objections, I plan on merging this later today.

miss-islington · 2023-06-13T21:02:23Z

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

miss-islington · 2023-06-13T21:19:47Z

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

miss-islington · 2023-06-13T21:19:48Z

Sorry @ericsnowcurrently, I had trouble checking out the 3.12 backport branch.
Please retry by removing and re-adding the "needs backport to 3.12" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 757b402ea1c2c6b925a55a08fd844b065b6e082f 3.12

For a while now, pending calls only run in the main thread (in the main interpreter). This PR changes things to allow any thread run a pending call, unless the pending call was explicitly added for the main thread to run.

bedevere-bot · 2023-06-13T21:55:59Z

GH-105752 is a backport of this pull request to the 3.12 branch.

For a while now, pending calls only run in the main thread (in the main interpreter). This PR changes things to allow any thread run a pending call, unless the pending call was explicitly added for the main thread to run.

ericsnowcurrently · 2023-06-14T00:02:22Z

FYI, I'm fixing the wasm buildbots.

) For a while now, pending calls only run in the main thread (in the main interpreter). This PR changes things to allow any thread run a pending call, unless the pending call was explicitly added for the main thread to run. (cherry picked from commit 757b402)

ericsnowcurrently added the skip news label May 23, 2023

bedevere-bot mentioned this pull request May 23, 2023

Subinterpreter Pending Calls Never Run #104812

Closed

ericsnowcurrently marked this pull request as ready for review May 23, 2023 20:35

ericsnowcurrently requested a review from rhettinger as a code owner May 23, 2023 20:35

bedevere-bot added the awaiting core review label May 23, 2023

ericsnowcurrently removed the request for review from rhettinger May 23, 2023 20:35

This comment was marked as outdated.

Sign in to view

ericsnowcurrently force-pushed the per-interpreter-pending-calls branch from d05d4a0 to fe6ecc4 Compare June 2, 2023 21:16

ericsnowcurrently requested a review from markshannon as a code owner June 2, 2023 21:16

ericsnowcurrently added 16 commits June 2, 2023 15:16

Clear each pending call when popping it.

6e46450

Call _Py_FinishPendingCalls() in Py_EndInterpreter().

3b18b7d

Factor out has_pending_calls().

f8b483f

Skip the calls_to_do check in _Py_FinishPendingCalls().

44df0bc

Factor out _PyEval_MakePendingCalls().

e2a0281

Explicitly restrict Py_MakePendingCalls() to the main thread.

61e5d3e

Factor out _make_pending_calls().

5af09f2

Add the mainthreadonly arg to _PyEval_AddPendingCall().

72cc242

Always use the main interpreter for Py_AddPendingCall().

88f7757

Add _PyRuntime.ceval.pending_mainthread.

d24fafb

Run per-interpreter pending calls in any thread.

1701fa3

Drop _Py_ThreadCanHandlePendingCalls().

c6cfaca

Expand the handle_eval_breaker comment.

dc11024

Do not require faulthandler for test.support.threading_helper.start_t…

6c3d06c

…hreads().

Add tests.

69ff9e6

Add a NEWS entry.

fdde46d

ericsnowcurrently force-pushed the per-interpreter-pending-calls branch from fe6ecc4 to fdde46d Compare June 2, 2023 21:19

ericsnowcurrently added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 2, 2023

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 2, 2023

Merge branch 'main' into per-interpreter-pending-calls

d9924b4

ericsnowcurrently added the needs backport to 3.12 bug and security fixes label Jun 7, 2023

ericsnowcurrently added 8 commits June 8, 2023 14:11

Merge branch 'main' into per-interpreter-pending-calls

93e61c5

Merge branch 'main' into per-interpreter-pending-calls

3e1bc1f

Drop a dead import.

7b8b8da

Clarify some comments.

90b3a1f

Add timeouts.

4f0068d

Implement the remaining tests.

d5d7b42

Drop prints.

37d41cc

Ignore the global variable.

fc25a85

markshannon reviewed Jun 13, 2023

View reviewed changes

Clarify comments.

177f161

ericsnowcurrently merged commit 757b402 into python:main Jun 13, 2023
19 checks passed

bedevere-bot removed the awaiting core review label Jun 13, 2023

ericsnowcurrently deleted the per-interpreter-pending-calls branch June 13, 2023 21:02

ericsnowcurrently added needs backport to 3.12 bug and security fixes and removed needs backport to 3.12 bug and security fixes labels Jun 13, 2023

miss-islington assigned ericsnowcurrently Jun 13, 2023

bedevere-bot removed the needs backport to 3.12 bug and security fixes label Jun 13, 2023

ericsnowcurrently mentioned this pull request Jun 14, 2023

gh-104812: Skip Pending Calls Tests if No Threading #105761

Merged

swtaarrs mentioned this pull request Jan 23, 2024

Move the eval_breaker to PyThreadState #112175

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-104812: Run Pending Calls in any Thread #104813

gh-104812: Run Pending Calls in any Thread #104813

ericsnowcurrently commented May 23, 2023 •

edited

This comment was marked as outdated.

bedevere-bot commented Jun 2, 2023

ericsnowcurrently commented Jun 6, 2023

markshannon left a comment

markshannon Jun 13, 2023

ericsnowcurrently Jun 13, 2023

markshannon Jun 13, 2023

ericsnowcurrently Jun 13, 2023

ericsnowcurrently Jun 13, 2023

markshannon Jun 13, 2023

ericsnowcurrently Jun 13, 2023

markshannon Jun 13, 2023

ericsnowcurrently Jun 13, 2023

markshannon Jun 13, 2023

ericsnowcurrently Jun 13, 2023

markshannon Jun 13, 2023

markshannon Jun 13, 2023

ericsnowcurrently Jun 13, 2023

markshannon Jun 13, 2023

ericsnowcurrently Jun 13, 2023

ericsnowcurrently Jun 13, 2023

ericsnowcurrently commented Jun 13, 2023

miss-islington commented Jun 13, 2023

miss-islington commented Jun 13, 2023

miss-islington commented Jun 13, 2023

bedevere-bot commented Jun 13, 2023

ericsnowcurrently commented Jun 14, 2023

gh-104812: Run Pending Calls in any Thread #104813

gh-104812: Run Pending Calls in any Thread #104813

Conversation

ericsnowcurrently commented May 23, 2023 • edited

This comment was marked as outdated.

bedevere-bot commented Jun 2, 2023

ericsnowcurrently commented Jun 6, 2023

markshannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericsnowcurrently commented Jun 13, 2023

miss-islington commented Jun 13, 2023

miss-islington commented Jun 13, 2023

miss-islington commented Jun 13, 2023

bedevere-bot commented Jun 13, 2023

ericsnowcurrently commented Jun 14, 2023

ericsnowcurrently commented May 23, 2023 •

edited