Allow callers to cancel their request to cancel the script #1085
Conversation
| @@ -79,7 +79,8 @@ async Task<ScriptOperationExecutionResult> ObserveUntilComplete( | |||
| { | |||
| if (scriptExecutionCancellationToken.IsCancellationRequested) | |||
| { | |||
| lastResult = await scriptExecutor.CancelScript(lastResult.ContextForNextCommand).ConfigureAwait(false); | |||
| // We don't want to cancel this operation as it is responsible for stopping the script executing on the Tentacle | |||
| lastResult = await scriptExecutor.CancelScript(lastResult.ContextForNextCommand, CancellationToken.None).ConfigureAwait(false); | |||
There was a problem hiding this comment.
The CancellationToken.None is moved up here.
There was a problem hiding this comment.
The reason for this PR is so that we can cancel the cancellation request, so that the cancellation request does not block Octopus Server from shutting down.
But this made me wonder, why is this not an issue for the non-resilient pipeline? Surely it would get told to cancel a running deployment when it shuts down too, right? I would have thought that would also block.
| @@ -34,37 +34,43 @@ Task<ScriptExecutionResult> ExecuteScript( | |||
| CancellationToken scriptExecutionCancellationToken); | |||
|
|
|||
| /// <summary> | |||
| /// Start the script. | |||
| /// | |||
There was a problem hiding this comment.
Why did we delete this summary? Yeah, it's simple, but it's better than none.
| /// <param name="scriptExecutionCancellationToken"></param> | ||
| Task<ScriptStatus?> CompleteScript(CommandContext commandContext, ITentacleClientTaskLog logger, CancellationToken scriptExecutionCancellationToken); | ||
| /// <param name="requestCancellationToken">Cancels the inflight request</param> | ||
| Task<ScriptStatus?> CompleteScript(CommandContext commandContext, ITentacleClientTaskLog logger, CancellationToken requestCancellationToken); |
There was a problem hiding this comment.
Just wondering, what's the consequence of killing a request to cancel?
I would have thought this would result in the tentacle potentially running the script after we have told the user that we are cancelled, right?
Is this going to be an issue? I.e., is the trade off of shutting down Octopus Server quicker worth it?
Background
Otherwise callers of the event driven script executor can't bail out on requests to cancel the script.
Which is preventing octopus from shutting down.
How to review this PR
Quality ✔️
Pre-requisites