Better error handling during wait*() and Run.__init__()
#118
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
PatrykGala
reviewed
Jan 13, 2025
PatrykGala
force-pushed
the
kg/run-error-handling-fixes
branch
from
b6e03cd to
46c08e5
Compare
PatrykGala
requested changes
Jan 14, 2025
It's hacky, but shouldn't be problematic for now
This applies to cases when we close a Run from a different thread. It's better to wait until the worker process terminates, as it makes it easier to print out a final error message in case of an error, instead of having it mixed together with logging from different threads.
In case of an error callback from ProcessLink, push the message to errors queue instead of calling `Run.terminate()` directly. This helps avoiding deadlocks.
kgodlewski
force-pushed
the
kg/run-error-handling-fixes
branch
from
46c08e5 to
f19ef1c
Compare
Also make `wait*` return `True` if all operations were processed, `False` on timeout
kgodlewski
force-pushed
the
kg/run-error-handling-fixes
branch
from
f19ef1c to
de8d69d
Compare
PatrykGala
approved these changes
Jan 14, 2025
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.
This PR addresses a problem, where it's impossible to react gracefully and consistently to errors that occur during
Run.__init__(), namely run creation.With our current design it's impossible for the user (the user being our CLI as well mind you) to handle trivial errors like invalid API key passed to
Run, if a custom error handler is registered.This change is on the functional and UX front.
What it does:
boolfrom_wait()and friends.Truemeans "everything was processed",Falsemeans: "timeout or run is now being closed"timeoutargument almost(*) properlyRun.close()andRun.terminate()block until the process actually terminates, if called from a different thread that initiated the close operation. This helps with race conditions / cluttering stdout with logs in order which is not meaningful.(*) almost, because we apply the timeout to a single iteration of the wait loop. A future PR will address that.