Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.4k
gh-104812: Run Pending Calls in any Thread#104813
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
gh-104812: Run Pending Calls in any Thread#104813
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
d05d4a0 tofe6ecc4Comparefe6ecc4 tofdde46dComparebedevere-bot commentedJun 2, 2023
🤖 New build scheduled with the buildbot fleet by@ericsnowcurrently for commitfdde46d 🤖 If you want to schedule another build, you need to add the🔨 test-with-buildbots label again. |
markshannon left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This seems fine, but I'm not an expert on this, so another review would be appreciated
Python/ceval.c Outdated
| * - 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
Python/ceval.c Outdated
| * | ||
| * 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wrote from experience. 😄 It wasn't obvious to me how to write tests that ran pending calls in specific threads. I'll elaborate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
Python/ceval.c Outdated
| * "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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Say, "on back edges in the control flow graph", rather than mentioning the specific bytecode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
| &&_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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
COMPUTE_EVAL_BREAKER is getting unwieldy. Any thoughts on how to clean it up? (for another PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Python/ceval_gil.c Outdated
| interp=_PyInterpreterState_Main(); | ||
| } | ||
| return_PyEval_AddPendingCall(interp,func,arg); | ||
| /* Legacy users of this API will continue to target the main thread. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe clarify?
This means the "the main thread of the main interpreter", not the "the main thread of the current interpreter"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
| staticinlinevoid | ||
| SIGNAL_PENDING_CALLS(PyInterpreterState*interp) | ||
| SIGNAL_PENDING_CALLS(struct_pending_calls*pending,PyInterpreterState*interp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Want to make this and the unsignal version less SHOUTY, as it isn't a macro anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also, the name "SIGNAL" is a bit confusing as this doesn't handle signals, but code around here does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why is it "meant strictly for internal use"? What bad things happen if a third party uses it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've clarified the note.
ericsnowcurrently commentedJun 13, 2023
Unless there are any objections, I plan on merging this later today. |
miss-islington commentedJun 13, 2023
Thanks@ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
miss-islington commentedJun 13, 2023
Thanks@ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
miss-islington commentedJun 13, 2023
Sorry@ericsnowcurrently, I had trouble checking out the |
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 commentedJun 13, 2023
GH-105752 is a backport of this pull request to the3.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 commentedJun 14, 2023
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 commit757b402)
…ythonGH-139117)Prior to 3.9, Py_AddPendingCall() would always run pending calls in the main interpreter, but then each interpreter got their own ceval state, and they were scheduled for any interpreter. InpythonGH-104813, this was undone, so Py_AddPendingCall() would always schedule for the main interpreter.(cherry picked from commit89ff88b)Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
…ythonGH-139117)Prior to 3.9, Py_AddPendingCall() would always run pending calls in the main interpreter, but then each interpreter got their own ceval state, and they were scheduled for any interpreter. InpythonGH-104813, this was undone, so Py_AddPendingCall() would always schedule for the main interpreter.(cherry picked from commit89ff88b)Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
…3.12 (GH-139117) (GH-139119)Document `Py_AddPendingCall()` change with subinterpreters in 3.12 (GH-139117)Prior to 3.9, Py_AddPendingCall() would always run pending calls in the main interpreter, but then each interpreter got their own ceval state, and they were scheduled for any interpreter. InGH-104813, this was undone, so Py_AddPendingCall() would always schedule for the main interpreter.(cherry picked from commit89ff88b)Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Uh oh!
There was an error while loading.Please reload this page.
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.)