Add task cancellation hooks into the build definition. #1242
Conversation
* Create a new EvaluateTaskConfig which gives us a bit more freedom over changign config options to EvaluateTask in the future. * Create adapted from old EvaluateTask to new EvaluateTask * Add hooks into signals class to register/remote a signal listener directly, rather than in an "arm" block. * Create TaskEvaluationCancelHandler to control the strategy of who/whom can cancel (sbt-server vs. sbt-terminal). * Create a null-object for the "can't cancel" scenario so the code path is exactly the same. This commit does not wire settings into the build yet, nor does it fix the config extractio methods.
* Expose new EvaluateTaskConfig throughout all the APIs * Create a key for cancellation configuration * Add default values for cancellation in GlobalPlugin * Create a test to ensure that cancellation can cancel tasks. * Deprecate all the existing mechanisms of evaluating tasks which use the EvaluateConfig API.
| /** cancels whatever this points at. */ | ||
| def cancel(): Unit | ||
| } | ||
| /** A handler for registering/remmoving listeners that allow you to cancel tasks. */ |
There was a problem hiding this comment.
this comment seems confusing. "An implementation of cancellation for a particular task" ? "handle cancellation for a task evaluation" ?
There was a problem hiding this comment.
I guess in fact it's "Implementations of this trait determine what will trigger cancel() on the TaskCancel provided to the start() method. This may be a signal, or a protocol request, or nothing."
|
If I understand this correctly, the problem this solves is that we can call cancel() in response to a protocol request instead of a signal. It looks fine for that, my only thought is that I found the doc comments sort of unclear. |
|
@havocp Yep, that's what it does. I've fixed the documentation comments as suggested. |
| * [[TaskEvalautionCancelHandler]] which is responsible for calling `cancel()` when | ||
| * appropriate. | ||
| */ | ||
| trait TaskCancel { |
There was a problem hiding this comment.
If this represents anything that can be canceled, why not call it CanCancel instead of including the word Task?
There was a problem hiding this comment.
I did call it CanCancel first. I'm not sure this is aimed to be a generic "cancellable" API, I think I want to tie it to the TaskEngine, but calling it TaskEngine also felt wrong.
Should I update the docs to be nicer with regard to that?
There was a problem hiding this comment.
What's wrong with a generic cancelable API? Settings are like generic setting API.
There was a problem hiding this comment.
I don't really have enough use-cases/design ideas right now to fully genericize. We'd end up with a half-a$$ed implementation of a generic API. I'd rather not have to maintain such a thing until we know what a cancellation API should look like.
There was a problem hiding this comment.
If not CanCancel I'd go for CancellableTask so it's a noun phrase.
There was a problem hiding this comment.
Yeah, it's not a CancellableTask, but a CancellableTaskEngine....
I can modify to that if you think the name is better.
| taskCancelStrategy := { state: State => | ||
| if(cancelable.value) TaskCancellationStrategy.Signal | ||
| else TaskCancellationStrategy.Null | ||
| }, |
There was a problem hiding this comment.
When Defaults.scala starts to make sense, you know we are on to something.
* Rename TaskCancel to RunningTaskEngine for clarity * Explicitly pass a state value in TaskCancellationStrategy * Update hooks to be immutable/safe.
|
LGTM |
|
TravisCI failure seems to be transient timeout death. |
Add task cancellation hooks into the build definition.
Add task cancellation hooks into the build definition.
This enables configurable strategies for non-CLI clients to register for all task evaluations.
easier to evolve in BC way)
Review by @eed3si9n & @havocp cc @skyluc