-
Notifications
You must be signed in to change notification settings - Fork 3
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 task timeout #52
Add task timeout #52
Conversation
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.
Should we have a testcase where multiple tasks with different timeouts are enqueued? Since the timeout is managed in the main process I feel we should be able to prove that all the timeouts are honored concurrently in a correct manner.
1eba78f
to
8e2d207
Compare
8e2d207
to
d9343aa
Compare
@Alex-Izquierdo from your last review I've changed the remaining |
Fixes #54
Obviously this needs an integration test, but I wanted to just get implementation structure now. It is functional, tested with the demo script.EDIT: added tests
There are some aesthetic changes we could make, like call it "timeout" instead of "cancel"... but obviously timeout uses the same mechanism as cancel. Maybe I'll just rename the exception to
DispatcherInterrupt
or something like that.Also, I'm relatively happy with the structure, although it took some thought. I implemented "delayed" task in the "main" part of the code. It's non-obvious that timeouts will go in the pool part, but the pool can gate on capacity, and timeouts don't start until the worker actually starts running it. And this is definitely something we want enforced in the parent, in general.