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

Use fatal library in path_srv, cert_srv and sciond #2208

Merged
merged 1 commit into from
Dec 12, 2018
Merged

Use fatal library in path_srv, cert_srv and sciond #2208

merged 1 commit into from
Dec 12, 2018

Conversation

sustrik
Copy link
Contributor

@sustrik sustrik commented Dec 7, 2018

This patch also moves logging the fatal condition into the failing
goroutine and makes servers wait for 1 second before exiting. This
way we can collect multiple fatal errors in the log in case the
first one is not the most informative one.


This change is Reviewable

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 6 of 6 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @sustrik)


go/godispatcher/main.go, line 61 at r2 (raw file):

	<-fatal.Chan()
	// Prometheus encountered a fatal error, thus we exit.
	log.Crit("Unable to listen and serve")

Those two lines above are no longer needed.


go/lib/fatal/fatal.go, line 15 at r2 (raw file):

provide

provides


go/lib/fatal/fatal.go, line 19 at r2 (raw file):

they

not needed


go/lib/fatal/fatal.go, line 59 at r2 (raw file):

	// Ask main goroutine to shut down the application.
	select {
	case fatalC <- struct{}{}:

TIL that works a on a nil channel.


go/lib/fatal/fatal.go, line 62 at r2 (raw file):

		// Block until the application shuts down.
		select {}
	case <-time.After(10 * time.Second):

10 is plenty, maybe use 5 or 2?

Copy link
Contributor Author

@sustrik sustrik left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker)


go/godispatcher/main.go, line 61 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Those two lines above are no longer needed.

Done.


go/lib/fatal/fatal.go, line 15 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…
provide

provides

Done.


go/lib/fatal/fatal.go, line 19 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…
they

not needed

Done.


go/lib/fatal/fatal.go, line 62 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

10 is plenty, maybe use 5 or 2?

Done.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@sustrik sustrik merged commit 4164178 into scionproto:master Dec 12, 2018
@sustrik sustrik deleted the fatal branch December 12, 2018 13:39
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