Close event connections when the server is gracefully shutting down.

[dylanahsmith]

@airhorns for review

Problem

When the TERM signal is sent to TERM, then it waits for existing connections to finish before exiting (https://github.com/macournoyer/thin/blob/v1.6.2/lib/thin/server.rb#L175). There is a 30 second timeout for most requests, but the /events endpoint uses stream :keep_open do |out| to keep the connection open for server sent events. As a result, a graceful shutdown can take over an hour to complete.

Solution

I couldn't find a simple hook in thin to close connections when the server is shutting down, but I ended up wrapping the Thin::Server#stop method to close the event connections.

I tested manually by sending the TERM signal to the thin processes in development after visiting a page which connected to the /events endpoint. Previously this would get stuck waiting until I close the page, but now it completes quickly.

[airhorns]

This will interrupt open, normal, not-long-running requests that are in the process of completing right?

If so, could we add a timeout or something like that that only closes connections if they are still open after the request timeout? Would that cause 30 seconds of "downtime" where we aren't accepting new connections but waiting for old ones to close? Probably. That's less than ideal also.

[airhorns]

I think the real solution would be a puma or unicorn style rolling restart between processes to load up new code but the in memory event bus architecture of Dashing prevents that.

[dylanahsmith]

This will interrupt open, normal, not-long-running requests that are in the process of completing right?

Nope, the connections array is one maintained by the dashing rather than thin or sinatra, so it will only close the connections for the events endpoint.

I think the real solution would be a puma or unicorn style rolling restart between processes to load up new code but the in memory event bus architecture of Dashing prevents that.

Yes, this is definitely not a zero downtime restart like we get from unicorn.

[airhorns]

Oh schweet,

[pushmatrix]

I'm on my phone and can't see it right now. I'm just in transit so
give me a sec to take a look

[dylanahsmith]

Sorry for merging before you got to see it @pushmatrix. I can revert or fix forward based on any feedback you have though.

[pushmatrix]

So my concern here is that some users of Dashing don't use thin. I
realize that it's still a hardcoded dependency so they'll have to
include it in anyway... I guess this override won't run if they use
something different, correct?

Dashing 2.0 will be a tad more agnostic for this.

[dylanahsmith]

So my concern here is that some users of Dashing don't use thin. I realize that it's still a hardcoded dependency so they'll have to include it in anyway...

Ya, I would have done this in statly, but I thought it was more appropriate here after noticing that thin was a hard coded dependancy. I can revert and move it into statly now to make that transition easier though.

I guess this override won't run if they use something different, correct?

Ya, it would override the method, but then the method would never get used if they aren't using Thin.

[dylanahsmith]

This didn't work when there was more than one connection, because the callback on line 74 deletes the connection from the array, which results in .each here skipping connections. I pushed commit 55f9093 to dup the array before iterating on it here.

[airhorns]

Dastardly mutable iterations. Nice fix.