Log shutdown gracefully

[mjip]

Closes #427

@beorn7

[beorn7]

Thanks for your contribution.

Unfortunately, this doesn't work as intended. The way the graceful shotdown is implemented right now is to close the socket. After that, web.Serve returns with on error (something like accept tcp [::]:9091: use of closed network connection).

If you try out and run the code with your PR, then hit CTRL-C, you will still see the error-level log message.

[mjip]

Thanks for picking that up! I added a check on the error message- unfortunately, the maintainers of the net package decided against exporting the errClosing error for use of closed network connections, so I couldn't check the returned error type- golang/go#4373 I also added a comment for context about this change.

[beorn7]

Hmm, I think relying on a certain substring in the error message is very brittle.

I'm not really an expert when it comes to HTTP serving in Go, but I assume the real problem is that we bluntly call l.Close() here:

pushgateway/main.go

Line 257 in 079d8d8

l.Close()

I have now done a bit of research, and my impression is that we should store a pointer to the http.Server in a variable and give it to the closeListenerOnQuit goroutine. The server is created here:

pushgateway/main.go

Line 198 in 079d8d8

err = web.Serve(l, &http.Server{Addr: *listenAddress, Handler: mux}, *webConfig, logger)

Then, instead of calling l.Close(), we could call http.Server.Shutdown. Which has two advantages: First, the error returned is now the exported http.ErrServerClosed. Second, we got an actual graceful shutdown for free, so we can get rid of this weird code:

pushgateway/main.go

Lines 200 to 203 in 079d8d8

	// To give running connections a chance to submit their payload, we wait
	// for 1sec, but we don't want to wait long (e.g. until all connections
	// are done) to not delay the shutdown.
	time.Sleep(time.Second)

The reason why things were done in this weird way is that it is really old code. The http.Server.Shutdown method was only added in Go1.8, and this Pushgateway code predates that Go version.

Does that make any sense?

[mjip]

Your suggestion made sense to me, and I prefer it as well. Let me know if this is what you had in mind.

[beorn7]

Code looks perfect. Thank you very much for removing this old wart.

I just have a tiny bit of nitpicking about a doc comment.

[beorn7]

Nit: End sentence with a period.

And, even more nitpicking, "log gracefully" might not really nail it. How about "In the case of a graceful shutdown, do not log the error."

[mjip]

Apologies- I did read the Go Code Review Comments document, but forgot about that convention. This has been changed.

[beorn7]

Looks perfect. Thank you very much.

[yejumihoutao]

Hello dear,sorry for desturbing you.
Are there any recent release that include this change?
@beorn7

[beorn7]

@yejumihoutao There hasn't been a lot happening since the last release. But it is quite long ago (1.4.1 happened in May), so I'll cut 1.4.2. Please review #432.

[yejumihoutao]

@beorn7
ok,thank you for answering me.