Should fcgi catch EPIPE?

Should fcgi catch EPIPE?

[Christian Neukirchen]

15:17:37 < stbuehler> chris2: hi! i've been told to ask you... i'd
like to know how to get rid of the EPIPE backtraces in my
fastcgi.crash.log :)
15:17:52 < stbuehler> fastcgi-spec: "When a Web server is not
multiplexing requests over a transport connection, the Web server can
abort a request by closing the request's transport connection." - so i
think rack should handle EPIPE somehow
15:17:56 < stbuehler> some backtraces from EPIPE:
http://paste.lighttpd.net/727

Any good reason to not catch EPIPE?

-- 
Christian Neukirchen  <chneukirchen@gmail.com>  http://chneukirchen.org

Re: Should fcgi catch EPIPE?

[James Tucker]

[-- Attachment #1: Type: text/plain, Size: 1896 bytes --]


On 23 Dec 2009, at 16:31, Christian Neukirchen wrote:

> 
> 15:17:37 < stbuehler> chris2: hi! i've been told to ask you... i'd
> like to know how to get rid of the EPIPE backtraces in my
> fastcgi.crash.log :)
> 15:17:52 < stbuehler> fastcgi-spec: "When a Web server is not
> multiplexing requests over a transport connection, the Web server can
> abort a request by closing the request's transport connection." - so i
> think rack should handle EPIPE somehow
> 15:17:56 < stbuehler> some backtraces from EPIPE:
> http://paste.lighttpd.net/727
> 
> Any good reason to not catch EPIPE?

At the top level - maybe - maybe not.

Consider this (awful pseudo code):

run lambda do |env|
  thing = Persistence.fetch(:a_thing)
  begin; lock = thing.acquire_lock; end until lock
  print thing.some_property
  thing.change!
  print thing.some_other_property
  lock.release
end

The slightly more correct version, would be:

run lambda do |env|
  begin
    thing = Persistence.fetch(:a_thing)
    begin; lock = thing.acquire_lock; end until lock
    print thing.some_property
    thing.change!
    print thing.some_other_property
  ensure
    lock.release
  end
end

I'm sure you get the principle of intent here. This is relevant because, in example 1 (how IME the bulk of rubyists would write their apps), the problem of unresolved locks is almost impossible to debug unless the persistence layer can give you some kind of trace on when / where the lock was acquired. If we've silenced EPIPE completely, then there's no indication of error to the application developer, and so many developers would take a long time to realise that example 2 is how they need to deal with the problem.

The question is, do we want to protect those folks (given that we're framework-like)?

> -- 
> Christian Neukirchen  <chneukirchen@gmail.com>  http://chneukirchen.org


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3679 bytes --]

Re: Should fcgi catch EPIPE?

[Christian Neukirchen]

James Tucker <jftucker@gmail.com> writes:

> If we've silenced EPIPE completely, then there's no indication of
> error to the application developer

I thought of something like:

      def self.send_headers(out, status, headers)
        out.print "Status: #{status}\r\n"
        headers.each { |k, vs|
          vs.split("\n").each { |v|
            out.print "#{k}: #{v}\r\n"
          }
        }
        out.print "\r\n"
        out.flush
+     rescue Errno::EPIPE
      end

      def self.send_body(out, body)
        body.each { |part|
          out.print part
          out.flush
        }
+     rescue Errno::EPIPE
      end

-- 
Christian Neukirchen  <chneukirchen@gmail.com>  http://chneukirchen.org

Re: Should fcgi catch EPIPE?

[Eric Wong]

Christian Neukirchen <chneukirchen@gmail.com> wrote:
> James Tucker <jftucker@gmail.com> writes:
> > If we've silenced EPIPE completely, then there's no indication of
> > error to the application developer
> 
> I thought of something like:
> 
>       def self.send_headers(out, status, headers)
>         out.print "Status: #{status}\r\n"
>         headers.each { |k, vs|
>           vs.split("\n").each { |v|
>             out.print "#{k}: #{v}\r\n"
>           }
>         }
>         out.print "\r\n"
>         out.flush
> +     rescue Errno::EPIPE
>       end
> 
>       def self.send_body(out, body)
>         body.each { |part|
>           out.print part
>           out.flush
>         }
> +     rescue Errno::EPIPE
>       end

To be pedantic, body#each (and headers#each) may also hit EPIPE by
themselves (body could be a pipe/socket itself) and the application
developer may want to know to track down the error with a backtrace.

But avoiding excessive logging/backtraces is a good idea if the
client initiated the disconnect, since it's a potential DoS
(and at the best case, makes a lot of noise).

The following diff is similar to what I did with the Unicorn::TeeInput
class since that can expose EOFError to the application, which (being
client initiated and out of the app's control) would just mean
unnecessary noise for anybody inspecting logs.

Warning: totally untested, possible typos/syntax errors

--- a/lib/rack/handler/fastcgi.rb
+++ b/lib/rack/handler/fastcgi.rb
@@ -53,11 +53,15 @@ module Rack
         env.delete "CONTENT_TYPE"  if env["CONTENT_TYPE"] == ""
         env.delete "CONTENT_LENGTH"  if env["CONTENT_LENGTH"] == ""
 
+        class ClientDisconnect < Errno::EPIPE
+        end
+
         begin
           status, headers, body = app.call(env)
           begin
             send_headers request.out, status, headers
             send_body request.out, body
+          rescue ClientDisconnect
           ensure
             body.close  if body.respond_to? :close
           end
@@ -67,21 +71,26 @@ module Rack
         end
       end
 
+      def self.print_safely(out, part, flush = false)
+        out.print part
+        out.flush if flush
+      rescue Errno::EPIPE => e
+        raise ClientDisconnect, "client disconnected", []
+      end
+
       def self.send_headers(out, status, headers)
-        out.print "Status: #{status}\r\n"
+        print_safely(out, "Status: #{status}\r\n")
         headers.each { |k, vs|
           vs.split("\n").each { |v|
-            out.print "#{k}: #{v}\r\n"
+            print_safely(out, "#{k}: #{v}\r\n")
           }
         }
-        out.print "\r\n"
-        out.flush
+        print_safely(out, "\r\n", true)
       end
 
       def self.send_body(out, body)
         body.each { |part|
-          out.print part
-          out.flush
+          print_safely(out, part, true)
         }
       end
     end
-- 
Eric Wong