Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

Use a common signals handler #633

Merged
merged 2 commits into from
May 6, 2015
Merged

Conversation

inercia
Copy link
Contributor

@inercia inercia commented May 6, 2015

Both the router and the DNS server use a similar signals handler, so this PR implements a common handler for them. The DNS server and the router implement methods like Stop() and Status() now, called from the handler when some specific signals are received...

Stop() error
}

func InitDefaultSignalsHandler(ss []SignalsReceiver) {

This comment was marked as abuse.

@rade rade self-assigned this May 6, 2015
@rade
Copy link
Member

rade commented May 6, 2015

Erhm. The signal handler kept the router main() alive. It's now in a goroutine, so doesn't. Hence weave dies on startup.

@rade
Copy link
Member

rade commented May 6, 2015

I see that in the nameserver code, DNSServer.Start() blocks. And there is a whole bunch of logic to accomplish that. A Start() method shouldn't block. And I see no compelling reason why you'd want dns server startup to block. other than to keep the main loop alive. the signal handler loop is a fine choice for the latter, imo.

@inercia
Copy link
Contributor Author

inercia commented May 6, 2015

@rade I have thought several times about renaming the Start()/Stop() methods in the DNSServer to ListenAndServe()/Shutdown(). Those names would describe much better what they do, and it would follow the same convention found in other places in the standard library... ListenAndServe() is usually a blocking method, the last method call in a main()function and, IMHO, much better than a signals handler...

If you agree, I will make StartSignalHandlers() a blocking function (so the router does not need to be changed) and I will leave the Start()/Stop()->ListenAndServe()/Shutdown() for a future PR...

@rade
Copy link
Member

rade commented May 6, 2015

ok, but then it can't be called *Start*SignalHandlers. SignalHandlerLoop?

@rade rade merged commit f723d5a into weaveworks:master May 6, 2015
@rade rade modified the milestone: 0.11.0 May 12, 2015
@inercia inercia deleted the weave-common-signals branch June 8, 2015 11:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants