search

braintree / manners

A polite Go HTTP server that shuts down gracefully.
MIT License
999 stars 103 forks source link

Ensures the FinishRoutine is only called once per instance #11

Closed etix closed 10 years ago

etix commented 10 years ago

This fixes a crash where the HTTP server might close the client connection during a transfer (via the chunkWriter for example) and might call Close() again after completion, resulting in a crash: sync: negative WaitGroup counter

kainosnoema commented 10 years ago

I ran into this problem too. Is the Close() function even the best place to hook into this? I ask because one related issue is that if the client doesn't close the connection, by default Go holds the connection open (doesn't call Close() and reuses it for subsequent requests). This means there could be a long period of time between when shutdown is initiated, and when Go finally (maybe) closes the connection(s).

It seems like what the start/finish routines should really be wrapping, are the actual requests from being served over the connection, and also probably sending "Connection: close" with the last request before shutting down. Otherwise there's no way to reliably shutdown in a reasonable amount of time.

kainosnoema commented 10 years ago

Also, keep in mind that currently manners doesn't even wait on the WaitGroup during shutdown, so all this is hard to test without the patch in #10.

lionelbarrow commented 10 years ago

@kainosnoema You're right, the current implementation conflates serving a request with serving a connection, which isn't correct in many situations.

@etix This pull request is a step forward but it doesn't fix the actual problem. If the WaitGroup is decremented due to Go closing a connection and re-opening it, Manners will incorrectly fail to block shutdowns in the mean time.

How would you guys feel about having the library wrap the http.HandlerFunc the user passes with an increment and decrement directly? Then the WaitGroup is directly constrained to a single logical request rather than a connection.

titanous commented 10 years ago

Just ran into this.

@lionelbarrow Using the WaitGroup within a Handler is vulnerable to race condition where a request can be received but the handler goroutine hasn't been started when the call to shut down the server occurs. The real hooks for this will be available in Go 1.3, so I think the best approach is to apply this patch and then wait for Go 1.3 and use the new hooks. What do you mean by "re-opening" a connection?

lionelbarrow commented 10 years ago

Good point @titanous. I was referring to re-using a TCP connection, which after reading the documentation more carefully, is separate from re-using a net.Conn. I'll merge this in, and when Go 1.3 comes out I'll take a look at rewriting manners to solve this problem.

etix commented 10 years ago

Since Go 1.3 is now out, a proper fix should be applied using ConnState.

lionelbarrow commented 10 years ago

Yep! Finally got to this today. Let me know what you think.

etix commented 10 years ago

Looks definitely better! I'll let you know if I find any issue.

Thanks!