Thread state handling improvements aka bug and crash fixes when using Tohil/Python from multiple Tcl interpreters #70
Conversation
This file contains 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
for every function of the tohil Tcl-side that does something with python such as exec'ing, eval'ing, importing, call'ing, don't just switch to the threadstate for the tcl interpreter's corresponding python interpreter but also keep track of it and switch it back afterwards.
* make tohil_setup_subinterp return the prior python thread state * extend calls to tohil_setup_subinterp in Tohil_Init to keep the prior thread state and prior to returning to restore it.
Thanks to @NasaGeek for identifying this problem, which is setting a static value in two data types that are used by multiple python subinterpreters. We simplify by getting rid of that and finding the tcl interpreter the same way we do elsehwere when we need it and have been given no direct handle to it.
we were still restoring the thread state too early in a lot of cases. rather than calling the return function but not returning it, grabbing the return, then restoring the thread state, then returning the return value of the return function, we add a PyThreadState parameter to each ot tohil_tcl_return, Tohil_ReturnTclError, Tohil_ReturnStartupExceptionToTcl, and Tohil_ReturnExceptionToTcl, and in all the functions that make use of those we grab the prior thread state when we swap the thread state to the python subinterpreter associated with the tcl interpreter and pass that value back to the aforementioned functions.
resuna
reviewed
Nov 17, 2021
NasaGeek
reviewed
Nov 17, 2021
Good catch! The fall-through path did not restore the subinterpreter. It now does. |
|
oops i didn't mean to close the PR |
Added a note to the comment for this function about returning the thread state and the non-obvious implicit thread-state-swap that Py_NewInterpreter() performs. Hat tip to @NasaGeek.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For every function of the tohil Tcl-side that does something with Python such as exec'ing, eval'ing, importing, call'ing, or invoking a Tcl/Python callback, don't just switch to the threadstate for that tcl interpreter's corresponding Python interpreter, but also keep track of what the threadstate is when those functions are first called, and switch it back right before they return.
To do this we