Recieve and handle ctrl + c and exit application #953

asahasrabuddhe · 2019-11-28T06:34:08Z

Fixes #945

We were correctly receiving ctrl+c signal and cancelling our application context. The missing piece of the puzzle was not listening for context cancellation and exiting the program.

Since ctrl+c is a non standard program termination, I have used a non zero exit code i.e. 1 for the moment. Please let me know if we want to give the user an option to set a exit code themselves in case of ctrl+c exit. We can then add an option in the App for that then.

codecov · 2019-11-28T06:34:49Z

Codecov Report

Merging #953 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #953      +/-   ##
==========================================
+ Coverage   73.36%   73.41%   +0.05%     
==========================================
  Files          32       32              
  Lines        2440     2445       +5     
==========================================
+ Hits         1790     1795       +5     
  Misses        540      540              
  Partials      110      110

Impacted Files	Coverage Δ
context.go	`94.32% <100%> (+0.16%)`	⬆️
app.go	`77.07% <0%> (+0.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2dc008d...c2569ee. Read the comment docs.

saschagrunert

LGTM.

I would not add another option for this.

AudriusButkevicius · 2019-11-28T07:54:23Z

As I explained, we should not do this. This takes away the opportunity from the user to clean up. We could add an option which when enabled cancels the context, but we should definitely not call os.Exit(1) ourselves.

If I have some sort of SQL transaction ongoing, this will just yank the control from my function and kill the process without me being able to roll it back.

coilysiren · 2019-11-28T07:56:27Z

@AudriusButkevicius & @ansrivas can you two have a conversation about what the ideal solution is here? This is one potential solution for #945.

asahasrabuddhe · 2019-11-28T07:57:06Z

@AudriusButkevicius I think the real problem here is the lack of a sensible default. For most use cases, os.Exit is enough. I think we can probably have a handler function that would exit by default but would give advanced users a chance to write their own implementation for cleaning their stuff up.

AudriusButkevicius · 2019-11-28T08:04:53Z

An option with os.Exit encourages people to design their applications poorly, not performing cleanup.

This library should be about argument parsing and based on the arguments calling functions, not about lifecycle management of application.

I suspect you won't find a library in other languages that does os.Exit unasked
What if people want to implement "Are you sure you want to cancel current operation?" prompt?

asahasrabuddhe · 2019-11-28T08:06:00Z

If we have a default, they can read about it in the documentation and override it! The implementation right now provides no way to do this.

asahasrabuddhe · 2019-11-28T08:06:45Z

Or we could simply have that handler print a message about intercepting the ctrl + c and request the user to override it!

AudriusButkevicius · 2019-11-28T08:14:24Z

I think the default should be to do nothing, as thats least surprising from the API perspective with a out-of-the-box available handler that does os.Exit that users have to setup.

Perhaps the default handler should cancel the context on interrupt, which if the user implemented his app as expected (propagating contexts all the way down), would lead to the same effect.

asahasrabuddhe · 2019-11-28T08:33:16Z

I think the default should be to do nothing, as thats least surprising from the API perspective with a out-of-the-box available handler that does os.Exit that users have to setup.

Perhaps the default handler should cancel the context on interrupt, which if the user implemented his app as expected (propagating contexts all the way down), would lead to the same effect.

I agree. Let me implement this.

…register-ctrl-c

asahasrabuddhe · 2019-11-28T08:36:46Z

@AudriusButkevicius Is this better now?

…register-ctrl-c

ansrivas · 2019-11-28T08:47:48Z

Perhaps the default handler should cancel the context on interrupt, which if the user implemented his app as expected (propagating contexts all the way down), would lead to the same effect.

This totally makes sense here as this was exactly the confusion btw in #945 - how to propagate a context further for a cleanup.

asahasrabuddhe · 2019-11-28T08:52:12Z

@ansrivas I think now you'd be able to do this:

app.InterruptHandlerFunc = func(c *cli.Context) {
	// received interrupt
        // cleanup
}

context.go

app.go

asahasrabuddhe · 2019-11-28T10:28:35Z

Hi @AudriusButkevicius I have made the necessary changes. Here's an example illustrating the use of the feature:

package main

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

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

func main() {
	app := &cli.App{
		Name:  "long",
		Usage: "this takes a long time to finish",
		Action: func(c *cli.Context) error {
			fmt.Println("working...")
			time.Sleep(10 * time.Second)
			return nil
		},
	}

	app.Flags = []cli.Flag{
		&cli.IntFlag{
			Name: "count",
		},
	}

	signalCount := 1
	app.InterruptHandler = func(c *cli.Context, cancelFunc context.CancelFunc) {
		go func() {
			<-c.Done()
			os.Exit(1)
		}()

		count := c.Int("count")
		if signalCount < count {
			fmt.Println("received interrupt")
		} else {
			fmt.Println("signal count exceeded, quitting")
			cancelFunc()
		}

		signalCount++
	}

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

In the above example, I am waiting for the interrupt signal to be received 5 times before the context is cancelled.

asahasrabuddhe · 2019-11-28T14:00:03Z

The CI is failing because gfmrun is not able to find app.InterruptHandler in the current codebase. All the other test cases have passed. Please consider merging this.

app.go

context.go

app.go

context.go

funcs.go

…register-ctrl-c

AudriusButkevicius · 2019-12-01T08:04:13Z

app.go

@@ -151,6 +156,10 @@ func (a *App) Setup() {
 		a.Action = helpCommand.Action
 	}

+	if a.InterruptHandler == nil {
+		a.InterruptHandler = CancelContextOnInterrupt
+	}


Should not do this, user might set it to null not to get the context cancelled.

Essentially, if this is null, we should not even try to setup signal handling, and leave it up to the user.

I think if it's set to nil, we should actually set it to the Noop handler. We can add that to the documentation as well. It's easier that way to not cause nil panics

AudriusButkevicius · 2019-12-01T08:07:00Z

app.go

+	// This function is automatically executed when the context is cancelled due to the program
+	// receiving an interrupt signal (Control+C). This function can be overridden to implement
+	// desired behaviour in such a scenario
+	InterruptHandler InterruptHandlerFunc


The function is executed when the application receives and interuption signal, what that does is configurable based on what the handler is doing.

If we change this to signal handler, we should change the comment quite heavily.

AudriusButkevicius · 2019-12-01T08:10:01Z

context.go

+			for {
+				<-signals
+				c.App.InterruptHandler(c)
+			}


for signal := range signals {}

AudriusButkevicius · 2019-12-01T08:10:44Z

context.go

@@ -37,13 +38,16 @@ func NewContext(app *App, set *flag.FlagSet, parentCtx *Context) *Context {

 	if c.Context == nil {
 		ctx, cancel := context.WithCancel(context.Background())
+		signals := make(chan os.Signal, 1)
+		signal.Notify(signals, syscall.SIGINT, syscall.SIGTERM)


Because we capture TERM, perhaps we should not call it an interrupt handler but call it signal handler. I did not see that we capture more than a single signal before. Also, because we capture multiple, perhaps we should pass the signal to the handler.

AudriusButkevicius · 2019-12-01T08:14:09Z

interrupt-handlers.go

+var ExitOnInterrupt InterruptHandlerFunc = func(ctx *Context) {
+	CancelContextOnInterrupt(ctx)
+	<-ctx.Done()
+	os.Exit(1)


I don't think we should cancel the context in this case.

To be honest, if we start passing down signals, I think default should be os.Exit on sigterm, cancel context on sigint

rliebz · 2019-12-01T20:05:07Z

I'd like to see testing around this. Specifically, I think cases should include tests for verifying that custom interrupt handlers (or signal handlers) are called when specified, tests for what happens with the defaults, and tests for what happens when a nil handler is specified.

coilysiren

clearing my review request for the moment - feel free to request me again later 🙏

asahasrabuddhe · 2019-12-02T10:37:04Z

I like the direction in which this is headed to. Let me further improve this :)

AudriusButkevicius · 2019-12-02T10:51:59Z

Sorry for changing the course so many times.

asahasrabuddhe · 2019-12-02T11:50:25Z

@AudriusButkevicius Please don't be sorry! I would change to course a million times to produce good software 🙂

coilysiren · 2019-12-08T08:17:21Z

Is this still relevant as of #975?

(I originally made this comment on #973, and #974 by mistake 😭)

burdiyan · 2019-12-12T11:54:42Z

I have struggled with developers not handling signals properly so much, so I'm really grateful that will come out of the box with cli.

I do strongly believe though, that cli must not run os.Exit. Simply return the error from app.Run() and then the user will do the os.Exit as they wish. And the commands themselves within the Action could just listen for context cancelation, giving the full control to the user in this case.

Basically the original approach (which is currently tagged as v2) worked pretty fine in a setup similar to this one:

app := &cli.App{
    Name:    "say",
    Version: "0.1.0",
    Commands: []*cli.Command{
        {
            Name:        "hello",
            Aliases:     []string{"hi"},
            Usage:       "use it to see a description",
            Description: "This is how we describe hello the function",
            Action: func(ctx *cli.Context) error {
                t := time.NewTicker(1 * time.Second)
                defer func() {
                    t.Stop()
                    fmt.Println("Gracefully shutting down...")
                    time.Sleep(2 * time.Second)
                }()

                fmt.Println("Timer started")
                for {
                    select {
                    case tt := <-t.C:
                        fmt.Println(tt.String())
                    case <-ctx.Done():
                        return ctx.Err()
                    }
                }
            },
        },
    },
}

if err := app.Run(os.Args); err != nil {
    fmt.Printf("%+v\n", err)
    os.Exit(1)
}

So this way everything works perfectly. The only thing missing though is normally you can force close the app pressing another ctrl+c, and to do that you have to signal.Stop(ch) on a channel that was listening signals.

I think this would be the best of both worlds: user control and handling signals by default.

AudriusButkevicius · 2019-12-12T13:55:20Z

I think this PR is dead. We've decided that the argument parsing library is not in the position to do signal handling, and instead allow users to pass a context to the cli which they can now cancel (by doing their own signal handling).

burdiyan · 2019-12-12T14:15:43Z

This would also be perfect. Having a way to integrate context easily is one of the reasons I came to cli from cobra (my PR spf13/cobra#893 there implementing this for some reason is not getting response from maintainers).

Recieve and handle ctrl + c and exit application

0b79a37

asahasrabuddhe requested a review from a team as a code owner November 28, 2019 06:34

Merge branch 'master' into register-ctrl-c

c44e5d3

saschagrunert previously approved these changes Nov 28, 2019

View reviewed changes

coilysiren previously approved these changes Nov 28, 2019

View reviewed changes

Merge branch 'master' into register-ctrl-c

24e9af7

Merge branch 'master' into register-ctrl-c

62b7e64

Provide HandlerFunction instead of os.Exit by default

1beff0f

Merge branch 'register-ctrl-c' of https://github.com/urfave/cli into …

bb6eb9c

…register-ctrl-c

asahasrabuddhe dismissed stale reviews from coilysiren and saschagrunert via bb6eb9c November 28, 2019 08:33

asahasrabuddhe requested review from saschagrunert and coilysiren November 28, 2019 08:36

Merge branch 'master' into register-ctrl-c

ed5c042

asahasrabuddhe requested a review from AudriusButkevicius November 28, 2019 08:37

asahasrabuddhe added 2 commits November 28, 2019 14:12

handle scenario when the interrupt handler is not defined

9f7199e

Merge branch 'register-ctrl-c' of https://github.com/urfave/cli into …

1e07723

…register-ctrl-c

AudriusButkevicius reviewed Nov 28, 2019

View reviewed changes

context.go Outdated Show resolved Hide resolved

context.go Show resolved Hide resolved

app.go Outdated Show resolved Hide resolved

app.go Outdated Show resolved Hide resolved

asahasrabuddhe added 3 commits November 28, 2019 15:47

code review fixes

096f56f

update example in manual

a1fc77c

generate toc

c229460

asahasrabuddhe requested a review from AudriusButkevicius November 28, 2019 14:01

AudriusButkevicius reviewed Nov 28, 2019

View reviewed changes

app.go Outdated Show resolved Hide resolved

context.go Show resolved Hide resolved

app.go Outdated Show resolved Hide resolved

context.go Outdated Show resolved Hide resolved

funcs.go Outdated Show resolved Hide resolved

coilysiren assigned asahasrabuddhe Nov 29, 2019

coilysiren and others added 8 commits November 28, 2019 19:04

Merge branch 'master' into register-ctrl-c

2835f8d

move signals channel and notify function call outside the closure

5f33fbd

add default interrupt handlers and add default functionality

26c179b

update gitignore

0bbdd53

update documentation to describe new interrupt handling behaviour

05220e2

Merge branch 'register-ctrl-c' of https://github.com/urfave/cli into …

be75958

…register-ctrl-c

fix docs

029d4d5

Merge branch 'master' into register-ctrl-c

10cec48

AudriusButkevicius reviewed Dec 1, 2019

View reviewed changes

coilysiren reviewed Dec 2, 2019

View reviewed changes

Merge branch 'master' into register-ctrl-c

c2569ee

rliebz mentioned this pull request Dec 4, 2019

v2 feature: Allow registering ctrl-c with the app #945

Closed

3 tasks

asahasrabuddhe closed this Dec 12, 2019

coilysiren deleted the register-ctrl-c branch December 25, 2019 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recieve and handle ctrl + c and exit application #953

Recieve and handle ctrl + c and exit application #953

asahasrabuddhe commented Nov 28, 2019

codecov bot commented Nov 28, 2019 •

edited

Loading

saschagrunert left a comment

AudriusButkevicius commented Nov 28, 2019 •

edited

Loading

coilysiren commented Nov 28, 2019

asahasrabuddhe commented Nov 28, 2019

AudriusButkevicius commented Nov 28, 2019

asahasrabuddhe commented Nov 28, 2019

asahasrabuddhe commented Nov 28, 2019

AudriusButkevicius commented Nov 28, 2019

asahasrabuddhe commented Nov 28, 2019

asahasrabuddhe commented Nov 28, 2019

ansrivas commented Nov 28, 2019 •

edited

Loading

asahasrabuddhe commented Nov 28, 2019 •

edited

Loading

asahasrabuddhe commented Nov 28, 2019

asahasrabuddhe commented Nov 28, 2019

AudriusButkevicius Dec 1, 2019

AudriusButkevicius Dec 1, 2019

asahasrabuddhe Dec 2, 2019

AudriusButkevicius Dec 1, 2019

AudriusButkevicius Dec 1, 2019

AudriusButkevicius Dec 1, 2019

AudriusButkevicius Dec 1, 2019

AudriusButkevicius Dec 1, 2019

AudriusButkevicius Dec 1, 2019

rliebz commented Dec 1, 2019

coilysiren left a comment

asahasrabuddhe commented Dec 2, 2019

AudriusButkevicius commented Dec 2, 2019

asahasrabuddhe commented Dec 2, 2019

coilysiren commented Dec 8, 2019

burdiyan commented Dec 12, 2019

AudriusButkevicius commented Dec 12, 2019

burdiyan commented Dec 12, 2019

Recieve and handle ctrl + c and exit application #953

Recieve and handle ctrl + c and exit application #953

Conversation

asahasrabuddhe commented Nov 28, 2019

codecov bot commented Nov 28, 2019 • edited Loading

Codecov Report

saschagrunert left a comment

Choose a reason for hiding this comment

AudriusButkevicius commented Nov 28, 2019 • edited Loading

coilysiren commented Nov 28, 2019

asahasrabuddhe commented Nov 28, 2019

AudriusButkevicius commented Nov 28, 2019

asahasrabuddhe commented Nov 28, 2019

asahasrabuddhe commented Nov 28, 2019

AudriusButkevicius commented Nov 28, 2019

asahasrabuddhe commented Nov 28, 2019

asahasrabuddhe commented Nov 28, 2019

ansrivas commented Nov 28, 2019 • edited Loading

asahasrabuddhe commented Nov 28, 2019 • edited Loading

asahasrabuddhe commented Nov 28, 2019

asahasrabuddhe commented Nov 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rliebz commented Dec 1, 2019

coilysiren left a comment

Choose a reason for hiding this comment

asahasrabuddhe commented Dec 2, 2019

AudriusButkevicius commented Dec 2, 2019

asahasrabuddhe commented Dec 2, 2019

coilysiren commented Dec 8, 2019

burdiyan commented Dec 12, 2019

AudriusButkevicius commented Dec 12, 2019

burdiyan commented Dec 12, 2019

codecov bot commented Nov 28, 2019 •

edited

Loading

AudriusButkevicius commented Nov 28, 2019 •

edited

Loading

ansrivas commented Nov 28, 2019 •

edited

Loading

asahasrabuddhe commented Nov 28, 2019 •

edited

Loading