Cli: v2 feature: Allow registering ctrl-c with the app

[ansrivas]

Checklist

• [x] Are you running the latest v2 release? The list of releases is here.
• [x] Did you check the manual for your release? The v2 manual is here
• [x] Did you perform a search about this problem? Here's the Github guide about searching.

Describe the bug

Thank you for the v2 release.

Expecting the application to handle the ctrl-c but it doesn't. So the question then becomes:

• How to cleanly register a ctrl-c handler with the current app? ( I saw in the code NewContext but the signal is not getting handled apparently?

To reproduce

Run the following snippet and press ctrl-c

package cli

import (
    "fmt"
    "log"
    "os"
    "time"

    "github.com/urfave/cli/v2"
)

func RealMain() {
    // ctx, cancel := context.WithCancel(context.Background())
    // defer cancel()

    app := &cli.App{
        Name:  "boom",
        Usage: "make an explosive entrance",
        Action: func(c *cli.Context) error {
            fmt.Println("boom! I say!")
            time.Sleep(time.Duration(time.Second * 10))
            return nil
        },
    }

    // c := make(chan os.Signal, 2)
    // signal.Notify(c, syscall.SIGINT, syscall.SIGTERM)
    // go func() {
    //  <-c
    //  fmt.Println("\r- Ctrl+C pressed in Terminal")
    //  os.Exit(0)
    // }()

    err := app.Run(os.Args)
    if err != nil {
        log.Fatal(err)
    }
}

Expected behavior

Expecting the application to stop.

Version of go

go version go1.13.3 linux/amd64

[lynncyrin]

Hi @ansrivas, thanks for the well written issue!

I'm not quite sure if this is a bug, or a documentation issue? 🔍 I'll add both labels for now.

I'm also marking this as help wanted so that someone can drop in and help figure it out!

[arch-mage]

Is there any reason to label this issue as feature? I think this should be labeled as a bug. Because almost all foreground processes that received an interrupt signal exits right away or do clean up and then exit.

[AudriusButkevicius]

I don't think it's a bug. Killing the application without giving users code a chance to clean up would definitely be a bug. What if the user wants to roll back a transaction or something else on interrupt?

At this point we require the users of the library handle termination themselves, as we are not sure what they want to do when that happens.

The purpose of this library is mostly parse arguments and direct them to functions, not the application lifecycle management.

[arch-mage]

Yes, the purpose of this library is mostly parse arguments and direct them to functions, not the application life cycle management. But this version right now is handling life cycle management by intercepting interrupt signal and by doing that it's doing something outside of it's scope of purpose.

[AudriusButkevicius]

The v2 version does not do that. I guess you are arguing that it should stay that way?

[rliebz]

@AudriusButkevicius there's either some form of life cycle management going on, or a bug in the behavior right now. Going for the most minimal example based on the issue description:

package main

import (
    "fmt"
    "os"
    "time"

    "github.com/urfave/cli/v2"
)

func main() {
    app := &cli.App{
        Action: func(c *cli.Context) error {
            fmt.Println("starting...")
            time.Sleep(time.Duration(time.Second * 10))
            fmt.Println("done")
            return nil
        },
    }

    // Pressing ctrl+C here does NOT interrupt execution
    app.Run(os.Args)

    // Pressing ctrl+C here DOES interrupt execution
    app.Action(nil)
}

I haven't dug into exactly why that's the case, but it is not what I would have expected the default behavior to be.

[rliebz]

Looks like the behavior was introduced in #840

Specifically:
https://github.com/urfave/cli/blob/2dc008dada7993557e593ebd18ad0d656a90b208/context.go#L38-L47

From the Go docs:

Notify disables the default behavior for a given set of asynchronous signals and instead delivers them over one or more registered channels.

So apparently we _are_ doing lifecycle management by default. For certain code paths, including app.Run, avoiding that signal.Notify call is not even possible.

While https://github.com/urfave/cli/pull/953 may be something we're interested in, I think the real resolution to this issue is to use the background context by default, and let users who want to manage signals themselves do it manually:

    if c.Context == nil {
-       ctx, cancel := context.WithCancel(context.Background())
-       go func() {
-           defer cancel()
-           sigs := make(chan os.Signal, 1)
-           signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
-           <-sigs
-       }()
-       c.Context = ctx
+       c.Context = context.Background()
    }

[lynncyrin]

☝️ FYI @asahasrabuddhe

[lynncyrin]

☝️ FYI @marwan-at-work too

[AudriusButkevicius]

So the first problem is that you are calling action, you should not do that, as thats a struct member used for registering the call back. You calling it is essentially just calling any go function and not related to the library in any way.

I am not sure what go's default signal haning behaviour is, but you seem to suggest there is some sort of handler.

I guess I did not expect that, and if there is already something, perhaps we should leave it as it is by default but provide additional means in the library to get context cancelled etc when this happens, but don't do that out of the box, as there is some niceness in that from a perspective of a library.

[rliebz]

So the first problem is that you are calling action

Sorry if that wasn't clear—Calling app.Action was just to illustrate that Go's default signal handling behavior is different than what app.Run does. Just time.Sleep(10 * time.Second) would have been a better example.

I am not sure what go's default signal haning behaviour is

From the docs:

By default, a synchronous signal is converted into a run-time panic. A SIGHUP, SIGINT, or SIGTERM signal causes the program to exit.

So there is a default behavior, and it is overridden by calling signal.Notify.

perhaps we should leave it as it is by default but provide additional means in the library to get context cancelled etc when this happens, but don't do that out of the box

I think this is the only reasonable path forward.

Reference: https://golang.org/pkg/os/signal/#hdr-Default_behavior_of_signals_in_Go_programs

[asahasrabuddhe]

I think then we should drop the PR I am working on and instead remove the signal.Notify call and let the default behaviour as implemented by Go continue. Advanced users can write the signal.Notify call themselves and implement desired behaviour.

If everyone agrees, please indicate by a 👍 on this post so that I can re-purpose the PR and make the changes.

[marwan-at-work]

@asahasrabuddhe @lynncyrin

Thanks for tagging me in on this. The current code is definitely a bug :) But let me expand a little more about how it can be nicely fixed:

The net/http library sends a Cancellation signal through its *http.Request.Context when a user terminates the connection.

It can easily be misleading for many Go developers to see a Context that doesn't propagate the cancellation as expected.

That said, I do see how this is a problem but instead of completely removing the code we can easily make it behave like many other CLI tools where the first ctrl+c will issue a cancellation signal, but the second one will issue a force-exit.

This can be accomplished by just adding two lines to the codebase in context.go:

Before:

defer cancel()
<-sigs

After:

<-sigs
cancel()
<-sigs
os.Exit(0)

We can also add some sane defaults, like warning the user that they need to hit ctrl+c again, or maybe just exiting after N milliseconds from the first ctrl+c

Furthermore, we can just let the CLI developer customize the behavior by accepting some opts such as "allow the first ctr+c to exit after N milliseconds" or "make the Context cancellation opt-in/out"

Something like:

&cli.App{
  ExitTimeout: time.Second,
  ContextCancellation: true,
}

Naming to be determined.

Why we can't handle signal.Notify on the user side:

I say all of this because the problem with letting the user handle signal.Notify in their main has no way of injecting cancellation in urfave/cli.Context :

Take this app for example

package main

import (
    "fmt"
    "log"
    "os"
    "time"

    "github.com/urfave/cli/v2"
)

func main() {
    ctx, cancel := context.WithCancel(context.Background())
    defer cancel()

    app := &cli.App{
        Name:  "boom",
        Usage: "make an explosive entrance",
        Action: func(c *cli.Context) error {
            someAPI.ExpensiveCall(c) // this will never get cancelled, even when we exit the application
            return nil
        },
    }

    c := make(chan os.Signal, 1)
    signal.Notify(c, syscall.SIGINT, syscall.SIGTERM)
    go func() {
        <-c
        fmt.Println("\r- Ctrl+C pressed in Terminal")
        os.Exit(0)
    }()

    err := app.Run(os.Args)
    if err != nil {
        log.Fatal(err)
    }
}

Notice the comment next to the someAPI.ExpensiveCall(c) call

[marwan-at-work]

One way to solve my code above, is to make a new channel and call signal.Notify and starting a context Cancellation Goroutine on ever cli Action. But that can get super messy

Alternate solution:

Maybe the BeforeAction function can return a new Context and the user might only write this logic once

Before:

type BeforeFunc func(*Context) error

After:

type BeforeFunc func(*Context) (context.Context, error)

This would be a breaking change so I imagine we can make a new name like BeforeFuncContext

This way a user can just do this once:

&cli.App{
  BeforeFuncContext(c *cli.Context) (context.Context, error) {
    ctx, cancel := context.WithCancel(context.Background())
    c := make(chan os.Signal, 2)
    signal.Notify(c, syscall.SIGINT, syscall.SIGTERM)
    go func() {
        <-c
        cancel()
        time.Sleep(time.Second) // custom behavior on the user side
        os.Exit(0)
    }()
    return ctx, nil
  }
}

This can make the user customize their signal cancellation behavior without altering the default behavior of just exiting on crl+c

[AudriusButkevicius]

I think we need a RunWithContext, or NewAppWithContext. The fact we push down "some" context to the user is cool but if the user has no control over it, it's a bit weird.

[marwan-at-work]

@AudriusButkevicius I like that idea as well 👍 -- I prefer RunWithContext so that users can still describe their API by a struct literal without having to user constructors. Plus, Run() is the thing that creates a context so that's perfect

[marwan-at-work]

I added a PR with @AudriusButkevicius's suggestion. Though I'm happy to change that PR up into any of the other suggestions 👍