From ac05e7035e1946b78ce4679548db7680aa01734c Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Tue, 17 Aug 2010 08:35:03 +0000 Subject: avoid EBADF with certain middlewares when proxying First off we use an FD_MAP to avoid creating redundant IO objects which map to the same FD. When that doesn't work, we'll fall back to trapping Errno::EBADF and IOError where appropriate. --- lib/rainbows.rb | 6 ++ lib/rainbows/dev_fd_response.rb | 4 +- lib/rainbows/response/body.rb | 8 ++- t/close-pipe-to_path-response.ru | 30 ++++++++++ t/t0032-close-pipe-to_path-response.sh | 101 +++++++++++++++++++++++++++++++++ 5 files changed, 147 insertions(+), 2 deletions(-) create mode 100644 t/close-pipe-to_path-response.ru create mode 100755 t/t0032-close-pipe-to_path-response.sh diff --git a/lib/rainbows.rb b/lib/rainbows.rb index 04d5ebc..2faf3c8 100644 --- a/lib/rainbows.rb +++ b/lib/rainbows.rb @@ -30,6 +30,12 @@ module Rainbows G = State.new(true, 0, 0, 5) O = {} class Response416 < RangeError; end + + # map of numeric file descriptors to IO objects to avoid using IO.new + # and potentially causing race conditions when using /dev/fd/ + FD_MAP = {} + FD_MAP.compare_by_identity if FD_MAP.respond_to?(:compare_by_identity) + # :startdoc: require 'rainbows/const' diff --git a/lib/rainbows/dev_fd_response.rb b/lib/rainbows/dev_fd_response.rb index 637bcc2..7f70b8e 100644 --- a/lib/rainbows/dev_fd_response.rb +++ b/lib/rainbows/dev_fd_response.rb @@ -14,7 +14,8 @@ class Rainbows::DevFdResponse < Struct.new(:app) # :stopdoc: - # + FD_MAP = Rainbows::FD_MAP + # make this a no-op under Rubinius, it's pointless anyways # since Rubinius doesn't have IO.copy_stream def self.new(app) @@ -37,6 +38,7 @@ class Rainbows::DevFdResponse < Struct.new(:app) headers = HeaderHash.new(headers) st = io.stat fileno = io.fileno + FD_MAP[fileno] = io if st.file? headers['Content-Length'] ||= st.size.to_s headers.delete('Transfer-Encoding') diff --git a/lib/rainbows/response/body.rb b/lib/rainbows/response/body.rb index cf14f08..2535374 100644 --- a/lib/rainbows/response/body.rb +++ b/lib/rainbows/response/body.rb @@ -30,6 +30,12 @@ module Rainbows::Response::Body # :nodoc: ALIASES = {} + FD_MAP = Rainbows::FD_MAP + + def io_for_fd(fd) + FD_MAP.delete(fd) || IO.new(fd) + end + # to_io is not part of the Rack spec, but make an exception here # since we can conserve path lookups and file descriptors. # \Rainbows! will never get here without checking for the existence @@ -41,7 +47,7 @@ module Rainbows::Response::Body # :nodoc: # try to take advantage of Rainbows::DevFdResponse, calling File.open # is a last resort path = body.to_path - path =~ %r{\A/dev/fd/(\d+)\z} ? IO.new($1.to_i) : File.open(path) + path =~ %r{\A/dev/fd/(\d+)\z} ? io_for_fd($1.to_i) : File.open(path) end end diff --git a/t/close-pipe-to_path-response.ru b/t/close-pipe-to_path-response.ru new file mode 100644 index 0000000..abc3a37 --- /dev/null +++ b/t/close-pipe-to_path-response.ru @@ -0,0 +1,30 @@ +# must be run without Rack::Lint since that clobbers to_path +class MyMiddleware < Struct.new(:app) + class Body < Struct.new(:body, :to_path) + def each(&block); body.each(&block); end + def close + c = body.respond_to?(:close) + ::File.open(ENV['fifo'], 'wb') do |fp| + fp.syswrite("CLOSING #{body.inspect} #{to_path} (#{c})\n") + end + body.close if c + end + end + + def call(env) + status, headers, body = app.call(env) + body.respond_to?(:to_path) and body = Body.new(body, body.to_path) + [ status, headers, body ] + end +end +use MyMiddleware +use Rainbows::DevFdResponse +run(lambda { |env| + io = IO.popen('cat random_blob', 'rb') + [ 200, + { + 'Content-Length' => ::File.stat('random_blob').size.to_s, + 'Content-Type' => 'application/octet-stream', + }, + io ] +}) diff --git a/t/t0032-close-pipe-to_path-response.sh b/t/t0032-close-pipe-to_path-response.sh new file mode 100755 index 0000000..e3d8f1b --- /dev/null +++ b/t/t0032-close-pipe-to_path-response.sh @@ -0,0 +1,101 @@ +#!/bin/sh +. ./test-lib.sh +if ! test -d /dev/fd +then + t_info "skipping $T since /dev/fd is required" + exit 0 +fi + +t_plan 16 "close pipe to_path response for $model" + +t_begin "setup and startup" && { + rtmpfiles err out http_fifo sub_ok + rainbows_setup $model + export fifo + rainbows -E none -D close-pipe-to_path-response.ru -c $unicorn_config + rainbows_wait_start +} + +t_begin "read random blob sha1" && { + random_blob_sha1=$(rsha1 < random_blob) +} + +t_begin "start FIFO reader" && { + cat $fifo > $out & +} + +t_begin "single request matches" && { + sha1=$(curl -sSfv 2> $err http://$listen/ | rsha1) + test -n "$sha1" + test x"$sha1" = x"$random_blob_sha1" +} + +t_begin "body.close called" && { + wait # for cat $fifo + grep CLOSING $out || die "body.close not logged" +} + +t_begin "start FIFO reader for abortive HTTP/1.1 request" && { + cat $fifo > $out & +} + +t_begin "send abortive HTTP/1.1 request" && { + rm -f $ok + ( + printf 'GET /random_blob HTTP/1.1\r\nHost: example.com\r\n\r\n' + dd bs=4096 count=1 < $http_fifo >/dev/null + echo ok > $ok + ) | socat - TCP:$listen > $http_fifo || : + test xok = x$(cat $ok) +} + +t_begin "body.close called for aborted HTTP/1.1 request" && { + wait # for cat $fifo + grep CLOSING $out || die "body.close not logged" +} + +t_begin "start FIFO reader for abortive HTTP/1.0 request" && { + cat $fifo > $out & +} + +t_begin "send abortive HTTP/1.0 request" && { + rm -f $ok + ( + printf 'GET /random_blob HTTP/1.0\r\n\r\n' + dd bs=4096 count=1 < $http_fifo >/dev/null + echo ok > $ok + ) | socat - TCP:$listen > $http_fifo || : + test xok = x$(cat $ok) +} + +t_begin "body.close called for aborted HTTP/1.0 request" && { + wait # for cat $fifo + grep CLOSING $out || die "body.close not logged" +} + +t_begin "start FIFO reader for abortive HTTP/0.9 request" && { + cat $fifo > $out & +} + +t_begin "send abortive HTTP/0.9 request" && { + rm -f $ok + ( + printf 'GET /random_blob\r\n' + dd bs=4096 count=1 < $http_fifo >/dev/null + echo ok > $ok + ) | socat - TCP:$listen > $http_fifo || : + test xok = x$(cat $ok) +} + +t_begin "body.close called for aborted HTTP/0.9 request" && { + wait # for cat $fifo + grep CLOSING $out || die "body.close not logged" +} + +t_begin "shutdown server" && { + kill -QUIT $rainbows_pid +} + +t_begin "check stderr" && check_stderr + +t_done -- cgit v1.2.3-24-ge0c7