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

Signal/Interrupt package #1392

Merged
merged 5 commits into from
Jun 29, 2018
Merged

Conversation

cfromknecht
Copy link
Contributor

Moves most of the contents in signal.go into it's own package named signal. This is done in preparation of having standalone watchtowers, so each daemon can share this code.

Notable changes:

  • handlers are now executed in LIFO order. Before we were registering closures that executed in order, though this allows us to have similar semantics as defer. The handlers in lnd.go have now been separated, and are registered after each Start().
  • handlers are now func() error instead of func(). All of the handlers we use are of this type, so it makes it cleaner even though we don't handle the errors during an interrupt.

@cfromknecht
Copy link
Contributor Author

One question I have is whether we even need AddInterruptHandler, instead of just preferring to defer the Stop() methods. If we also defer the log message stating that shutdown is complete at the beginning of lndMain, everything should already be in lifo order.

@cfromknecht
Copy link
Contributor Author

I added two commits that impl the defer approach, lmk which you all prefer

Roasbeef
Roasbeef previously approved these changes Jun 27, 2018
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! We'll be able to use this in a few other projects we have in mind.

One question: what's the rationale behind removing the interrupt handlers in favor of using regular defers everywhere?

signal/signal.go Outdated
@@ -12,33 +12,30 @@ import (

var (
// interruptChannel is used to receive SIGINT (Ctrl+C) signals.
interruptChannel chan os.Signal
interruptChannel = make(chan os.Signal, 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition! Should've been this way all along.

@cfromknecht
Copy link
Contributor Author

Rationale for using defer: the current system executes FIFO order, which is fine since only two functions are registered (one of which is composite, and for the most part, executes Stop in LIFO order of resource creation). Instead of reimplementing a LIFO queue, I chose defer since it's built into the language, is consistent with all other teardown paradigms in lnd, and results in less overall code.

@cfromknecht
Copy link
Contributor Author

can squash the last two commits if rationale is sound

@Roasbeef
Copy link
Member

👍

@cfromknecht
Copy link
Contributor Author

cfromknecht commented Jun 28, 2018

squashed commits and rebased on master

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💫

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants