Add call to wg.Wait() before returning from Serve(), fix server tests #10
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
http.Serve does not wait for requests to finish, so we have to wait for the GracefulServer's WaitGroup to reach zero, or the process will exit and in-flight routines will be killed. Signed-off-by: Evan Owen <kainosnoema@gmail.com>
|
This was a pretty egregious error on my part. I introduced this bug in this commit 4 months ago and basically broke the library. I apologize. Thanks a lot for catching this. |
lionelbarrow
added a commit
that referenced
this pull request
Apr 17, 2014
Add call to wg.Wait() before returning from Serve(), fix server tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Though the readme states clearly that "the call to server.ListenAndServe will stop blocking when all the requests are finished", this doesn't seem to be the case. I've put together multiple tests, and at least in Go 1.2, as soon as the listener is closed and
Accept()starts returning an error,http.Servereturns with that error immediately. Is it possible this is new behavior in Go 1.2?In any case,
mannersisn't callingwg.Wait()anywhere, so the process exits and all in-flight routines are killed. Essentially, it's currently doing nothing, as far as I can tell. The tests inserver_test.gopass because the process is still running when the test completes, meaning the goroutines eventually finish even after the server has shut down, even though in most environments they would be killed.This patch adds a call to
wg.Wait()before returning fromServe()when the listener has been closed, and fixes the test to actually check whether the request has completed before the server shuts down. Tested and working well with Go 1.2.