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

net/http: Server.Serve returns an error when the Listener is closed #11219

Open
willfaught opened this Issue Jun 15, 2015 · 12 comments

Comments

Projects
None yet
4 participants
@willfaught
Contributor

willfaught commented Jun 15, 2015

net/http.Server.Serve returns an error when the Listener argument is closed, which means you cannot gracefully stop the server (no error returned). It should instead check if the Listener is closed and return nil if so.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 15, 2015

Graceful shutdown involves more than that. Have you seen the various packages implementing graceful shutdown http://godoc.org/?q=graceful ? The mostly use the ConnState mechanism. See also the graceful shutdown history in #4674.

I'm not sure anything is required here. Any code doing graceful shutdown will have its own Listener type that's keeping track of state anyway.

@willfaught

This comment has been minimized.

Contributor

willfaught commented Jun 15, 2015

By graceful, I meant not returning the error returned from Listener.Accept. There should be some way to stop a server without having to deal with an error. Since there is no Server.Close, we have to close the Listener, but Server.Serve treats the error returned from the next Accept call as a normal error, instead of a signal to stop, which shouldn't be exposed to the caller.

@willfaught willfaught changed the title from net/http: Server.Serve does not handle a closed Listener gracefully to net/http: Server.Serve returns an error when the Listener is closed Jun 15, 2015

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 15, 2015

The behavior of those functions is frozen. Even if we wanted to change the behavior, the net.Listener interface doesn't document what "closed" means. You can implement your own net.Listener interface which implements graceful shutdown, or use one of the existing ones.

If anything, we can document the status quo more about the behavior of Serve and ListenAndServe's return values.

@bradfitz bradfitz added this to the Go1.5Maybe milestone Jun 15, 2015

@willfaught

This comment has been minimized.

Contributor

willfaught commented Jun 15, 2015

The problem isn't what the Listener is returning, it's what Server.Serve does with it. It should return nil when the Listener is Closed (for whatever Closed means; it doesn't matter, as you pointed out).

I can understand why this might be frozen, since lots of existing code assumes Serve (and ListenAndServe) never returns nil, although arguably that's not the language's (and thus Go 1's) problem, since an error can always be nil. I suppose a new Server.ServeBetter is out of the question? :)

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 15, 2015

Yes, that's out of the question. We're not changing or adding behavior here when there are already ways to do what you want.

@willfaught willfaught closed this Jun 15, 2015

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 15, 2015

We can still document this better, though.

@bradfitz bradfitz reopened this Jun 15, 2015

@willfaught

This comment has been minimized.

Contributor

willfaught commented Jun 19, 2015

Currently net.errClosing isn't exported, which is what is returned by net.Listener.Accept for TCP connections, so there's no clean way to check whether an Accept error is due to it being closed. You have to do a strings.HasSuffix(err.Error(), "use of closed network connection") check. It would be great if errClosing were exported so this check wasn't so fragile.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jun 19, 2015

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 19, 2015

Whether errClosing is exported or not doesn't change the fact that the net.Listener interface (http://golang.org/pkg/net/#Listener) shipped in Go 1 and doesn't document any well-known error values from its Close method. We could expand the documentation at this point, but we'd have to do so very carefully with lax wording so we don't change the rules on people or the expectations retroactively.

@willfaught

This comment has been minimized.

Contributor

willfaught commented Jun 23, 2015

It seems to me that exporting the error would be backward compatible. No existing programs would behave differently.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 23, 2015

It's more complicated than making one byte uppercase for the reasons explained earlier. And anything once exported has to be maintained and has to have its semantics locked-in. Which means we have to know what those semantics are, they need to be documented, they need to be tested, etc. Currently the return values of net.Listener.Close are undefined, as they always have been.

In any case, this is a documentation bug about net/http. Feel free to open a new bug or use the proposal process if you want to see net.Listener changed.

@rsc

This comment has been minimized.

Contributor

rsc commented Jun 29, 2015

Too late for Go 1.5.

@rsc rsc modified the milestones: Unplanned, Go1.5Maybe Jun 29, 2015

@ghost ghost referenced this issue May 27, 2016

Merged

Fix SIGHUP #165

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment