Skip to content
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

Make Task context aware #19

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

inkel
Copy link
Contributor

@inkel inkel commented Oct 4, 2022

Implements #6.

This is a continuation of #18, where we made Job context aware. At the time of opening this PR we have the following situation:

  • NestedTask only passes down the context to its children. It doesn't check in its state if the context is done or not; perhaps we could make it that it only calls its children Poll method only if the context is not done.
  • ShellTask is ignoring the context completely. We could change it to either create the command using exec.CommandContext and ignore the context on each polling iteration, or checking if the context is done when polling and call Process.Kill on the running process now stops the process if the context is cancelled.
  • CallbackTask is passing a custom context with a 1s timeout when calling the callback function. I believe it would be better if we just pass the context directly or if we add the timeout but using the given context as its parent instead of a new context.Background now it passes down the Poll context argument. In addition, the timeout could be configurable instead of hardcoded. I'm still not sure if the callback should be called if the context was done 🤔

Opening as a draft while we discussed the points mentioned above.

inkel added 4 commits October 4, 2022 10:16
Signed-off-by: Leandro López (inkel) <inkel.ar@gmail.com>
Signed-off-by: Leandro López (inkel) <inkel.ar@gmail.com>
Signed-off-by: Leandro López (inkel) <inkel.ar@gmail.com>
Signed-off-by: Leandro López (inkel) <inkel.ar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant