Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MYST-148 gracefully close on Interrupt, sigterm and sighup #107

Merged
merged 6 commits into from Jan 24, 2018

Conversation

Projects
None yet
4 participants
@shroomist
Copy link
Contributor

commented Jan 21, 2018

is this enough for graceful termination, or do we need a more elaborate system?

call sarah connor when terminator comes
Signed-off-by: Andrej N <jazzaiman@gmail.com>

@shroomist shroomist requested review from ignasbernotas, tadovas, donce and Waldz Jan 21, 2018

connectionManager,
httpApiServer,
}
sigterm := make(chan os.Signal, 2)
signal.Notify(sigterm, os.Interrupt, syscall.SIGTERM, syscall.SIGHUP)
go sarahConnor(sigterm, cmd)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 21, 2018

Member

Factory should be responsible 1 responsibility - for factoring, but not running

This comment has been minimized.

Copy link
@shroomist

shroomist Jan 21, 2018

Author Contributor

alright, this is now in Run()

@shroomist shroomist force-pushed the feature/MYST-148-sigterm-handle branch from ae0334e to 860dd40 Jan 21, 2018

sigtermListener goes at cmd.Run()
Signed-off-by: Andrej N <jazzaiman@gmail.com>

@shroomist shroomist force-pushed the feature/MYST-148-sigterm-handle branch from 860dd40 to 9c25221 Jan 21, 2018

@@ -67,6 +70,10 @@ type CommandRun struct {

//Run starts Tequilapi service - does not block
func (cmd *CommandRun) Run() error {
sigterm := make(chan os.Signal, 2)

This comment has been minimized.

Copy link
@donce

donce Jan 22, 2018

Contributor

Why 2, isn't 1 enough?

}
fmt.Println("Good bye")
os.Exit(1)
}

This comment has been minimized.

Copy link
@donce

donce Jan 22, 2018

Contributor

Closing subroutines and exiting is already implemented in mysterium.client.go/newStopHandler - can we re-use that somehow?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 22, 2018

Member

It's a bit different. newStopHandler creates handler which is called after user types quit int CLI

This comment has been minimized.

Copy link
@donce

donce Jan 22, 2018

Contributor

It's hooked up differently, but the action it does on exit is duplicated:

err := cmd.Kill()
if err != nil {
	fmt.Printf("Unable to kill one of subroutines %q\n", err.Error())
}
fmt.Println("Good bye")
os.Exit(1)
if err := cmdRun.Kill(); err != nil {
	return err
}

os.Exit(0)
return nil

What bothers me is that these two duplications already differs - one returns exit 1 and prints outs "Good bye", another returns 0 without text.

We can easily DRY these - to move them to a separate method on CommandRun or to extend CommandRun.Kill with these duplications.

@@ -98,3 +105,13 @@ func (cmd *CommandRun) Kill() error {

return nil
}

func sigtermListener(terminator chan os.Signal, cmd *CommandRun) {

This comment has been minimized.

Copy link
@donce

donce Jan 22, 2018

Contributor

Function should be named as a verb, not as a noun - maybe listenSigterm suites better?

<-terminator
err := cmd.Kill()
if err != nil {
fmt.Printf("Unable to kill one of subroutines %q\n", err.Error())

This comment has been minimized.

Copy link
@donce

donce Jan 22, 2018

Contributor

os.Stderr should be re-used for consistency, just like this:

fmt.Fprintln(os.Stderr, err)
fmt.Printf("Unable to kill one of subroutines %q\n", err.Error())
}
fmt.Println("Good bye")
os.Exit(1)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 22, 2018

Member

Exit(1) mean program ending with errorNo=1, success graceful stop should end with errorNo=0

This comment has been minimized.

Copy link
@donce

donce Jan 22, 2018

Contributor

agree - we are already doing os.Exit(0) in mysterium_client.go

cmd.terminator works for server and client
Signed-off-by: Andrej N <jazzaiman@gmail.com>
@@ -110,8 +110,15 @@ func (cmd *CommandRun) Wait() error {
return cmd.vpnServer.Wait()
}

func (cmd *CommandRun) Kill() {
func (cmd *CommandRun) Kill() (err error) {

This comment has been minimized.

Copy link
@donce

donce Jan 23, 2018

Contributor

You're not really using named result variable - you can just leave error.

added sigterm handler for cli
Signed-off-by: Andrej N <jazzaiman@gmail.com>
go waitTerminatorSignals(sigterm, command)
}

func waitTerminatorSignals(terminator chan os.Signal, command Command) {

This comment has been minimized.

Copy link
@shroomist

shroomist Jan 23, 2018

Author Contributor

for those who don't know -- this function is Sarah Connor

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 23, 2018

Member

I would like to call it - waitForLastJudgementDay

@donce

donce approved these changes Jan 23, 2018

@shroomist

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2018

@donce, is it better now?

@Waldz

Waldz approved these changes Jan 24, 2018

@shroomist shroomist merged commit 388881b into master Jan 24, 2018

@donce donce deleted the feature/MYST-148-sigterm-handle branch Mar 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.