Fix Timeout-minutes for Whole Composite Action Step #599
Conversation
This file contains hidden or 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
ethanchewy
commented
Jul 20, 2020
| @@ -273,30 +279,7 @@ private async Task RunStepAsync(IStep step) | |||
| step.ExecutionContext.Result = Common.Util.TaskResultUtil.MergeTaskResults(step.ExecutionContext.Result, step.ExecutionContext.CommandResult.Value); | |||
| } | |||
|
|
|||
| // Fixup the step result if ContinueOnError. | |||
| if (step.ExecutionContext.Result == TaskResult.Failed) | |||
There was a problem hiding this comment.
We don't need this since we'll explicitly disallow Continue on error for each composite step for now.
ethanchewy
commented
Jul 20, 2020
ethanchewy
commented
Jul 20, 2020
ericsciple
reviewed
Jul 22, 2020
| @@ -60,10 +60,12 @@ public interface IExecutionContext : IRunnerService | |||
|
|
|||
| bool EchoOnActionCommand { get; set; } | |||
|
|
|||
| IExecutionContext JobExecutionContext { get; set; } | |||
There was a problem hiding this comment.
Should we expose Root { get; } instead?
ericsciple
reviewed
Jul 22, 2020
| @@ -254,7 +258,7 @@ public void RegisterPostJobStep(IStep step) | |||
| DictionaryContextData inputsData, | |||
| Dictionary<string, string> envData) | |||
| { | |||
| step.ExecutionContext = Root.CreateChild(_record.Id, step.DisplayName, _record.Id.ToString("N"), scopeName, step.Action.ContextName, logger: _logger); | |||
| step.ExecutionContext = Root.CreateChild(_record.Id, step.DisplayName, _record.Id.ToString("N"), scopeName, step.Action.ContextName, logger: _logger, cancellationTokenSource: _cancellationTokenSource); | |||
There was a problem hiding this comment.
Would it make more sense for the interface to pass CancellationToken.CreateLinkedTokenSource(_cancellationTokenSource)
ericsciple
approved these changes
Jul 22, 2020
fabianishere
pushed a commit
to fabianishere/runner
that referenced
this pull request
Aug 24, 2020
* Exploring child Linked Cancellation Tokens * Preliminary Timeout-minutes fix * Final Solution for resolving cancellation token's timeout vs. cancellation * Clean up + Fix error handling * Use linked tokens instead * Clean up * one liner * Remove JobExecutionContext => Replace with public Root accessor * Move CreateLinkedTokenSource in the CreateCompositeStep Function
fabianishere
pushed a commit
to fabianishere/runner
that referenced
this pull request
Sep 23, 2020
* Exploring child Linked Cancellation Tokens * Preliminary Timeout-minutes fix * Final Solution for resolving cancellation token's timeout vs. cancellation * Clean up + Fix error handling * Use linked tokens instead * Clean up * one liner * Remove JobExecutionContext => Replace with public Root accessor * Move CreateLinkedTokenSource in the CreateCompositeStep Function
AdamOlech
pushed a commit
to antmicro/runner
that referenced
this pull request
Jan 28, 2021
* Exploring child Linked Cancellation Tokens * Preliminary Timeout-minutes fix * Final Solution for resolving cancellation token's timeout vs. cancellation * Clean up + Fix error handling * Use linked tokens instead * Clean up * one liner * Remove JobExecutionContext => Replace with public Root accessor * Move CreateLinkedTokenSource in the CreateCompositeStep Function
TingluoHuang
pushed a commit
that referenced
this pull request
Apr 21, 2021
* Exploring child Linked Cancellation Tokens * Preliminary Timeout-minutes fix * Final Solution for resolving cancellation token's timeout vs. cancellation * Clean up + Fix error handling * Use linked tokens instead * Clean up * one liner * Remove JobExecutionContext => Replace with public Root accessor * Move CreateLinkedTokenSource in the CreateCompositeStep Function
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.
The timeout-minutes token set in the workflow file for an individual composite action is correctly processed with the CancellationToken for the ExecutionContext of the whole composite action step but for some reason my composite action cannot differentiate between cancellation and timing out
In this PR, we fix that. We also remove unnecessary code as well.
Example Cancel: https://github.com/ethanchewy/testing-actions/runs/891636687
Example of 1 min. Timeout: https://github.com/ethanchewy/testing-actions/runs/891641094?check_suite_focus=true