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.
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Set terminate Option for user #985
base: main
Are you sure you want to change the base?
Add Set terminate Option for user #985
Changes from 4 commits
f4f04c7
4e89d15
1b2ba60
6ab7d68
f36515e
3b55b65
c186d86
c60cede
a9b35dd
80b0a79
4de71a5
c1420ea
3ab84c5
b5e917f
b1c9d29
28cd0ac
fd4f11c
9f81bb1
936e022
f75e180
aec8564
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
In session.Run(...) above on line 58, we don't do anything on termination?
The issue is that there's a potential error race. If the user calls 'SetTerminate()' and then a non terminate error was thrown from session.Run() the user will not know it was a non termination related error (unless they look at the string).
Can we detect termination from session.Run? If so, we should rename session_terminated to session_terminate_set and have a second variable for "session_terminated" for if we actually hit the termination case (or if we call ThrowErrorIfSessionTerminated, as that will catch the non session.Run terminate cases).
This way we separate requesting termination from hitting termination, and IsTerminated() should only return true if we hit termination. This does mean that IsTerminated() will return false after SetTerminate() is called, and won't return true until a function is called that checks for termination. This could be a problem.
@baijumeswani can review my thinking also.
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.
that's an interesting question. I believe the condition you are mentioning is rare, if the session is already terminated then it might be possible that the error might be different because of it being in terminated state. Also, if the error occurs because of some other reason, it should be produced in another run when the processing happens in non-terminated state and can be caught at that time.