Cancel and restart long running process on file change by using context.WithCancel(..) #60
Conversation
Task are now called in a goroutine meaning the order of completion is not guaranteed.
andreynering
added
the
type: enhancement
watch.go
Outdated
| break | ||
| } | ||
| // Use of goroutines means task order is not guaranteed. | ||
| // Not sure if task order is a requirement. (jackmordaunt) |
There was a problem hiding this comment.
Running them in parallel makes sense to me. @andreynering?
watch.go
Outdated
| // Not sure if task order is a requirement. (jackmordaunt) | ||
| go func(a string) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| taskCancellers[a] = cancel |
There was a problem hiding this comment.
I suggest we create the context and update taskCancellers[a] outside of the concurrent go function. Modifying a map concurrently without a mutex is subject to data-race conditions or worse.
There was a problem hiding this comment.
After some thought... Could we also actually simplify this just use the same context (instead of taskCancellers map), and cancel all tasks through the same cancel function?
There was a problem hiding this comment.
Great idea to use a single context. Updated the pull request to do just that.
watch.go
Outdated
| } | ||
| // Use of goroutines means task order is not guaranteed. | ||
| // Not sure if task order is a requirement. (jackmordaunt) | ||
| go func(a string) { |
There was a problem hiding this comment.
You don't need to pass along a, you could capture it instead.
for _, a := range args {
a := a // capture range variable
...
}This is relevant to change due to my next comment on line 23.
There was a problem hiding this comment.
Curious, are there any downsides to passing along the variable other than memory allocations?
watch.go
Outdated
| @@ -41,11 +47,21 @@ loop: | |||
| select { | |||
| case <-watcher.Events: | |||
| for _, a := range args { | |||
There was a problem hiding this comment.
could we just loop through taskCancellers directly? If it's set in a non-concurrent manner, we don't need as many checks as here.
watch.go
Outdated
| @@ -41,11 +47,21 @@ loop: | |||
| select { | |||
| case <-watcher.Events: | |||
| for _, a := range args { | |||
| if err := e.RunTask(context.Background(), Call{Task: a}); err != nil { | |||
There was a problem hiding this comment.
As long as taskCancellers is modified concurrently, this is subject to data-race, and some tasks might (theoretically) be started after ok returns false.
Removed unnecessary map of contexts in place of a single context. This avoids concurrent map access and redundant iteration.
I accidentally removed the goroutine surrounding the initial task executions. Added it back in this commit.
|
With 1175c01, task outputs noisy 'error' messages when using watch flag: |
Removed comment. Capturing range variable instead of passing it to groutine.
|
@JackMordaunt This should do the trick. Print only if it's not a context error: switch err {
case context.Canceled, context.DeadlineExceeded:
// supress error
default:
e.println(err)
}It's also missing args assigning: for _, a := range args {
a := a // this is important to prevent goroutine using the value of the next iteration |
Added function to check type of error.
|
@JackMordaunt Great work, thank you very much! |
Tasks are now called in a goroutine meaning the order of completion is not guaranteed.
I'm not sure if task order is a requirement.
The reason they have to be called in a
goroutineis because long running tasks would blockwatchTasks(..), such that it doesn't read-in file change events.The
goroutinesdon't leak because thecontext.CancelFunccancels the currently executing Task, causingExecutor.RunTask(..)to return, thus exiting thegoroutine.I tested it locally and it appears to work correctly: long running processes are cancelled and re-started when a file change event occurs.
Resolves #60