Updated interface with support for idle connections

[gwatts]

This version basically extends the work listed in #21.

It adds more tests, documentation, support for passing in non wrapped connections into Serve(), supports TLS, etc.

It also changes the API to basically exactly match the http package. The Server type not embeds http.Server which means all of that servers public properties and methods are easily accessible and the GracefulServer ends up being just a thin layer around it.

Matching the http package signatures makes for a much easier transition to using the manners package. I also added top level functions which eliminate the need to directly instantiate a GracefulServer directly at all for the common case where you just want to start a server. It then literally becomes a s/http.ListenAndServe/manners.ListenAndServe/ substitution.

It retains all the work done in #21, which basically includes being able to shutdown when idle keep-alive connections are the only connections left connected. eliminates race conditions in tests, etc. This version also passes go lint and uses a dynamic port number in test runs instead of being fixed to 7000.

As the interface isn't backwards compatible it would make sense to be reticent to adopt it, but I thought I'd put it out there before I fork it under a different name for my own use.

[klizhentas]

Nice job!

[gwatts]

Thanks @klizhentas :smile:

@lionelbarrow any thoughts on this PR, or #21 ?

[lionelbarrow]

This is fantastic. Let me review it more closely today.

[klizhentas]

I've been also looking at the implementation here:

https://github.com/rcrowley/go-tigertonic/blob/go1.3/server.go#L55

Note that the major difference is that the implementation above keeps track of the idle connections to close them all. I wonder what @lionelbarrow and @gwatts think about it.

[gwatts]

I actually thought about tracking all connections, and an earlier version did just that.. In the end I liked not having a global map of connections as a) No global map means no mutex contention on every request start/finish b) no overhead of maintaining that map and c) no memory overhead either, other than the bit of metadata we wrap every connection with now.

It's a tradeoff, but I liked the balance of a reasonably clean/fast shutdown without the performance impact on every request vs a more aggressive shutdown with the tracking required.

Maybe there's a better approach?

[klizhentas]

@gwatts

With your implementation, can we potentially miss connections that were transitioned to the Idle state before we set the closed to true? In this case we can exit without closing the idle connections. I wonder if that is true and if it matters.

[gwatts]

@klizhentas yes; that's the main compromise atm (i meant to add a comment into the code to that effect) - It allows the server to exit once all connections are idle, but does not close those that were idle prior to close being called - the assumption being that normally the next thing that happens is that your code exits, implicitly closing the connections anyway. If that's not the case, then that's not so good.

It would be nice to come up with a way to explicitly close those connections on close without requiring a server-scoped map. Would be nice if the net/http package provided a channel to signal "close idle" connections, of course ;-)

We could spawn a goroutine in the wrapped listener for each incoming connection that monitored a global channel to cause it to close the connection if its http state is currently idle. The downside to that is that we then have the 8k (as of Go 1.3) stack overhead of that goroutine per connection.

No doubt there's other options available too, but ran out of time to think about them the weekend I was working on this

[klizhentas]

@gwatts @lionelbarrow I anticipate this PR to be merged as it's a really usefull improvement and we've built some additional features on top of it already here:

https://github.com/mailgun/manners

[gwatts]

Cool - I was planning on working on another round of updates to that branch in a week or so when I get a little bit of time

[klizhentas]

@lionelbarrow @gwatts Mind looking at the changes I've done to support passing file descriptors from one server to another here?

https://github.com/mailgun/manners/compare/gwatts:master...master

I wonder what your thoughts are. I'm using this feature to achieve downtime-less restarts of binaries. So it may be useful to incorporate it into manners as well. Ping me if you are interested, so we can hack on this together.

[lionelbarrow]

Definitely I'll take a look after I get the code @gwatts submitted merged in. I've been chipping away at merging this in this branch: https://github.com/braintree/manners/tree/non-backwards-compat

[horkhe]

@lionelbarrow are you going to merge this to master any time soon? The master is lagging far behind. Thanks.