Re: Handling closed clients

[Eric Wong <normalperson@yhbt.net>]

Andrew Hobson <ahobson@gmail.com> wrote:
> We have a service that clients use to upload files. We have a couple
> of clients that are on slow links and so their upload times out. We
> get errors in the logs that I'd like to get rid of.

Those clients should really be hitting nginx, first.

> I was hoping that the recent commit
> 24b9f66dcdda44378b4053645333ce9ce336b413 would help us, but it does
> not. After digging in a bit, I have some ideas about why and a patch
> I'd like comments on.

Bubbling a socket level error during an application dispatch is tricky
because the application might not be prepared to handle it.

Unfortunately this error is unique to unicorn since it implements
streaming input.

Fwiw, wrapping the app Unicorn::PrereadInput middleware may help in
this situation if you're dealing with a buggy local client.

> The commit above attempts to ignore exceptions of types EOFError,
> Errno::ECONNRESET, Errno::EPIPE, and Errno::ENOTCONN. However, that
> doesn't address our issue because our exception comes when trying to
> write the response. It is a generic IOError and thus is not handled.

IOError usually means an attempt to use the socket when it was already
closed (possibly after it hit ECONNRESET/EPIPE/ENOTCONN).

The only place we close the client socket where it might be visible to a
Rack app is in the eof! method of StreamInput.  Based on what I'm
reading, this is what's happening.

However, lately, I'm thinking merely calling .shutdown on the socket is
sufficient (patch below), and the close just confuses things.

> In addition, I believe that change is unsafe because I believe client
> code could raise those exceptions. Think about a client with a
> database connection: that could also raise e.g. EOFError. I believe
> unicorn needs to check the status of the client connection instead of
> the type of exception that is raised.

The Rack application should _always_ be trapping exceptions it
generates, including DB.  Where we log "app error" is only to tell the
app author to fix their code and prevent a buggy app from completely
breaking a worker.

This also means the app needs to abort application processing if
env["rack.input"] operations return EOFError (Unicorn::ClientShutdown).

> The patch below has two new test cases: one where the client code
> raises an EOFError and one that replicates the condition we see where
> the client socket is closed before the response can be written.

Your patch is badly whitespace mangled so I can't apply it.
Anyways, I think closing the socket while app dispatch is running
is sufficient to avoid IOError (you'll end up hitting ENOTCONN
instead, I think).

diff --git a/lib/unicorn/stream_input.rb b/lib/unicorn/stream_input.rb
index c8a4240..d4aa5f9 100644
--- a/lib/unicorn/stream_input.rb
+++ b/lib/unicorn/stream_input.rb
@@ -141,7 +141,6 @@ private
     # raised EOFError on us.
     if @socket
       @socket.shutdown
-      @socket.close
     end
   ensure
     raise Unicorn::ClientShutdown, "bytes_read=#{@bytes_read}", []
_______________________________________________
Unicorn mailing list - mongrel-unicorn@rubyforge.org
http://rubyforge.org/mailman/listinfo/mongrel-unicorn
Do not quote signatures (like this one) or top post when replying