Use an isolated clone of the Session in workunit handlers to shield against cancellation #11721
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
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
…d be associated with a Session but not canceled by it. # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
|
The commits are useful to review independently. |
Eric-Arellano
approved these changes
Mar 17, 2021
Comment on lines
+1398
to
+1402
| fn session_isolated_shallow_clone(py: Python, session_ptr: PySession) -> CPyResult<PySession> { | ||
| with_session(py, session_ptr, |session| { | ||
| PySession::create_instance(py, session.isolated_shallow_clone()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Generally, I am curious if we'll be able to move free functions like this onto methods defined on PySession? Totally okay to not do in this PR - it's a followup project I want to explore to see if it further simplifies our FFI.
There was a problem hiding this comment.
Yes, we probably can. The with_$x context managers add a bunch of noise, and are much less central than calling allow_threads, which we should try to do in nearly all cases.
Merged
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.
Problem
As described in #11715, streaming workunit handler callbacks currently use the same
Sessionobject as is used in the foreground because they need access to the metrics generated there. But this means that they have the same UI settings as the foreground, and when the foreground is cancelled, they are too.Solution
Split the
Sessionobject intoSessionHandleandSessionStateto allow for a shallow clone of theSessionthat is isolated in terms of cancellation and UI.Then, use an
isolated_shallow_clonein theStreamingWorkunitHandlerso that it can continue to consume metrics and execute work (without a UI) after the client has gone away.Result
Fixes #11715.
[ci skip-build-wheels]