Shutdown socket writes before close

[tilgovi]

Calling shutdown(2) with SHUT_WR before close(2) ensures that a FIN
packet is sent before any RST packet. Doing so ensures that clients
and proxies get a clean shutdown signal when the server terminates the
connection while an upload is pending.

Fix #872

[tilgovi]

Only lightly tested using tcpdump. I observe a FIN packet being sent before an RST (if any is sent at all). Without this patch, only the RST is sent.

Test instructions after patch here: https://gist.github.com/tilgovi/9021a996bdaf21595f12
Before patch, no FIN is sent.

[tilgovi]

As far as I know this patch should be fine, but I can't tell if it's possible that util.close is called on a socket-like object that doesn't have a shutdown method. I think it's not, and that every worker that uses this call provides something supporting this API. But just in case, I'll wait for someone else to have a look.

AFAIK there is no problem to ever send a shutdown before close like this. To be extra safe though, maybe I should do two separate try/catch so that close is always called.

[benoitc]

shouldn't we wait for read right after?

[tilgovi]

You mean add a lingering close to gunicorn thread @c-nichols raised at the nginx forum and linked over here in #872?

[benoitc]

yeah, the only problem I see is how to handle the normal case: all have been received then we close. In that case we don't need to wait for the final EOF since it will block.

[tilgovi]

I think if we do this we will have to have a linger pool somewhere and
select on it. If we get back readable but zero bytes we close. I don't
think it is a problem if we don't close immediately, especially if we allow
options for configuring this behavior.
On Sep 9, 2014 12:04 AM, "Benoit Chesneau" notifications@github.com wrote:

In gunicorn/util.py:

@@ -271,6 +271,7 @@ def set_non_blocking(fd):

def close(sock):
try:

•    sock.shutdown(socket.SHUT_WR)
  

yeah, the only problem I see is how to handle the normal case: all have
been received then we close. In that case we don't need to wait for the
final EOF since it will block.

—
Reply to this email directly or view it on GitHub
https://github.com/benoitc/gunicorn/pull/874/files#r17284959.

[benoitc]

That's a good idea. This pool can be checked from time to time using a dedicated thread or something like it.

Not sure if it's totally relevant, but here is how I check if a socket is still connected in socketpool:
https://github.com/benoitc/socketpool/blob/master/socketpool/util.py#L84-L144

[benoitc]

so yes the idea would be to shutdown the socket on write like you did and put in a linger pool where it will checked on read until it returns . Then we will be able to close the socket. Maybe that work could be started along the work on #792 .

[benoitc]

@tilgovi should we do that for 19.2 or do you want to postpone it for 20.0 in october?

[tilgovi]

Let's postpone. At least, I have no time to work on it now.
On Sep 22, 2014 6:19 AM, "Benoit Chesneau" notifications@github.com wrote:

@tilgovi https://github.com/tilgovi should we do that for 19.2 or do
you want to postpone it for 20.0 in october?

—
Reply to this email directly or view it on GitHub
#874 (comment).

[benoitc]

ok :) postponing it for 19.3

[benoitc]

While working on #962 I was wondering if we couldn't do the following with the current design to make sure that the sockets are killed;

1. Put closed sockets in the main worker poller or a dedicated poller/ worker with a close callback associated.
2. On read or after a time we definitely close them

It could be done in the main thread. Or if we are worrying about the time it could take we could have a linger thread/worker.

Thoughts?

[benoitc]

I was thinking about this issue and wonder if couldn't solve it with such design:

• in a linger pool all sockets are put in an event loop using the selector API
• each time we come back in the accept loop we check if a socket is available for read in the event loop, if yes we then definitely close it using the shutdown method followed by the close method.

I think it could work, but will add an extra time when accepting a connection. Maybe we should maintain a thread? Or just put the linger pool inside the arbiter?

Also how do we handle the gevent and eventlet workers? We have little control on them using their server object ( I wonder if we shouldn't provide our own server impl at some point...) . Maybe we could ignore them for the next release?

Let me know what you think about that design anyway.

[benoitc]

superseded by #1025 all discussions now happen in that ticket.