watch: interrupt ongoing task

[stelcheck]

Use case: I have a task that starts a server in development mode. I would like to use watch to interrupt the ongoing build task before starting the new one.

I assume this would be the desired behavior for long-running build tasks as well.

[andreynering]

Hi @stelcheck,

So, Task actually kill the previous long running command before running it again, BUT it doesn't wait the previous one to finish before it runs the new one. I think this may be causing the issue you're having.

This is unfortunate, but it's due limitation of the Go context package (which is asynchronously). I'll have to study how to improve that.

[stelcheck]

That would indeed cause problems, at the very least intermittently.

The issue I am seeing though seems to be a bit different. Either:

1. The subprocess is killed with SIGKILL or another more exotic signal
2. Logs stop being outputted while the child process is terminating

I'd be happy to see if I can fix this myself if you can give me a pointer as to where to look in the code.

[stelcheck]

Actually, I am now convinced the task is not killed: once I send a signal to the task command, all instances of the application receive it at once.

 task run -w
task: Started watching for tasks: run
INFO[0000] Logger system set                             forceColors=true format=text
DEBU[0000]                         ___
DEBU[0000]     o__        o__     |   |\
DEBU[0000]    /|          /\      |   |X\
DEBU[0000]    / > o        <\     |   |XX\
INFO[0000] Setting up metrics system                     subpath=/metrics
INFO[0000] Setting up discovery system                   address="127.0.0.1:8500" scheme=http
INFO[0000] Setting up HTTP Server system                 address="127.0.0.1:8080" prefix=/
INFO[0000] Goal server is up and running
INFO[0000] Logger system set                             forceColors=true format=text
DEBU[0000]                         ___
DEBU[0000]     o__        o__     |   |\
DEBU[0000]    /|          /\      |   |X\
DEBU[0000]    / > o        <\     |   |XX\
INFO[0000] Setting up metrics system                     subpath=/metrics
INFO[0000] Setting up discovery system                   address="127.0.0.1:8500" scheme=http
INFO[0000] Setting up HTTP Server system                 address="127.0.0.1:8080" prefix=/
INFO[0000] Goal server is up and running
INFO[0000] Logger system set                             forceColors=true format=text
DEBU[0000]                         ___
DEBU[0000]     o__        o__     |   |\
DEBU[0000]    /|          /\      |   |X\
DEBU[0000]    / > o        <\     |   |XX\
INFO[0000] Setting up metrics system                     subpath=/metrics
INFO[0000] Setting up discovery system                   address="127.0.0.1:8500" scheme=http
INFO[0000] Setting up HTTP Server system                 address="127.0.0.1:8080" prefix=/
INFO[0000] Goal server is up and running


INFO[0165] Received signal interrupt, shutting down
INFO[0165] Stopping Goal server
INFO[0153] Received signal interrupt, shutting down
INFO[0153] Stopping Goal server
INFO[0165] Tearing down discovery system
INFO[0165] Tearing down metrics system
INFO[0165] Tearing down logger system
INFO[0153] Tearing down discovery system
INFO[0153] Tearing down metrics system
INFO[0165] Goal server stopped
INFO[0174] Received signal interrupt, shutting down
INFO[0174] Stopping Goal server
INFO[0153] Tearing down logger system
INFO[0153] Goal server stopped
INFO[0174] Tearing down discovery system
INFO[0174] Tearing down metrics system
INFO[0174] Tearing down logger system
INFO[0174] Goal server stopped
task: Failed to run task "run": context canceled
task: Failed to run task "run": context canceled

You can see the task has be started 3 times, and when I CTRL-C the three instances get killed altogether.

[stelcheck]

OK, think I got the gist of it. The issue seems to be that canceling the context does not equate with interrupting the process. The signal is simply propagated through the process tree.

[andreynering]

@stelcheck It's by design that sending a SIGINT to a process also send it to all subprocesses. This is a Unix thing and we don't have control about it.

But restarting a long-running task once a file changes (--watch), well, we could definitely do better here, even it's tricky. We should probably get rid of using Go's context package in this specific case (because cancellation is asynchronous) and manually kill all processes.

That'd require some refactor, and maybe even changes to mvdan/sh.

[smyrman]

You can still use context for cancellation, you just need to keep track-of and wait for the goroutines you start to complete. E.g. an example design assuming an executor was initialized (and not called) with the tasks it was going to run, and assuming the executor keeps track of it's sub-tasks before returning from Run.

func watcher(ctx context.Context, changes <-chan struct{}, exe task.Executor) error {
    for {
        var wg sync.WaitGroup
        subCtx, cancel := context.WithCancel(ctx)
        wg.Add(1)
        go func() {
            err := exe.Run(subCtx)
            cancel()
            if err != nil{
                return err
            }
            wg.Done()
        }()
        select {
            case <-changes:
                // some of the files we are watching changed...
                cancel()
                wg.Wait()
            case <-ctx.Done():
                // parent context cancelled (e.g. Ctrl+C).
                cancel()
                wg.Wait()
                return ctx.Error()
        }
    }
}

PS! this is just a proof-of-concept for the pattern itself; not expecting the actual code to look like this.

[smyrman]

PS! I had a brief look at the current design; it's a bit odd to set the context in the Executor struct. As per the context package recommendations, context should always be passed as the first parameter to a function (and then passed along for each sub-call, so it can e.g. be modified/replaced etc).

[andreynering]

It's a bit odd to set the context in the Executor struct. As per the context package recommendations, context should always be passed as the first parameter to a function (and then passed along for each sub-call, so it can e.g. be modified/replaced etc).

@smyrman Yeah, I know. I already starting moving it to parameters, but didn't do in all places yet.

[frederikhors]

@andreynering maybe an inspiration from https://github.com/cortesi/modd can help?

[andreynering]

@frederikhors Thanks for the suggestion.

Idea: Perhaps we could watch for the PID of the previous command to know when it's killed.

[adamkissvcc]

Here is another inspiration: https://github.com/githubnemo/CompileDaemon

Watches files, runs a build command, and launches a run command after the build is successful. Has very graceful shutdown handling with -graceful-kill set to true (with force kill after a timeout).

[sisp]

I'd also like to run a dev server while watching for changes to rebuild some other dependencies that the dev server doesn't watch itself. +1 for waiting for a graceful shutdown before re-running the ongoing task.