[PATCH 0/2] FreeBSD-related fixes (affects Linux users, too)

[PATCH 0/2] FreeBSD-related fixes (affects Linux users, too)

[Eric Wong]

[PATCH 1/2] test_server: avoid multiple workers for dead parent check
A minor test patch I had queued on my FreeBSD VM but never pushed.

[PATCH 2/2] wbuf: avoid corrupted large responses with zero-copy
This is a major fix, and likely to affect some Linux users with
large responses and fast clients.

Will push a new 1.3.1 release soon.

[PATCH 1/2] test_server: avoid multiple workers for dead parent check

[Eric Wong]

From: EW <e+fbsd@80x24.org>

This test is less reliable when there are multiple workers as the
second worker may not be ready to detect a dead parent.
This is still a possible race if the master dies very quicklly
before a worker is fully setup.
---
 test/test_server.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/test/test_server.rb b/test/test_server.rb
index bd38ce3..69babb3 100644
--- a/test/test_server.rb
+++ b/test/test_server.rb
@@ -302,7 +302,7 @@ class TestServer < Testcase
   end
 
   def test_mp_dead_parent
-    pid, host, port = new_mp_server
+    pid, host, port = new_mp_server(1)
     wpid = nil
     run_client(host, port) do |res|
       wpid ||= res.body.to_i
@@ -334,7 +334,7 @@ class TestServer < Testcase
     c.close
   end
 
-  def new_mp_server(nr = 1)
+  def new_mp_server(nr = 2)
     ru = @ru = tmpfile(%w(config .ru))
     @ru.puts('a = $$.to_s')
     @ru.puts('run lambda { |_| [ 200, {"Content-Length"=>a.size.to_s},[a]]}')
@@ -342,7 +342,7 @@ class TestServer < Testcase
     cfg = Yahns::Config.new
     host, port = @srv.addr[3], @srv.addr[1]
     cfg.instance_eval do
-      worker_processes 2
+      worker_processes nr
       GTL.synchronize { app(:rack, ru.path) { listen "#{host}:#{port}" } }
       logger(Logger.new(File.open(err.path, "a")))
     end
-- 
EW

[PATCH 2/2] wbuf: avoid corrupted large responses with zero-copy sendfile

[Eric Wong]

From: Eric Wong <e+fbsd@80x24.org>

This bug is noticeable on a amd64 FreeBSD 9.2 VM, and possible under
Linux, too.  This happens as a zero-copy sendfile implementation means
pages queued for transmission by the sendfile system call should not be
modified at any point after the sendfile syscall is made.

To prevent modification, we replace the temporary file with a new one.
This has a similar effect as truncate and can still prevent a dirty
flush in cases when a client consumes the response fast enough.

This reverts the misguided ade89b5142bedbcf07f38aa062bfdbfcb8bc48d3
commit ("wbuf: hack to avoid response corruption on FreeBSD")

Note: this bug was finally fixed because I finally noticed this flaw
in a different (non-Ruby, non-HTTP) server of mine.
---
 lib/yahns/wbuf.rb | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/lib/yahns/wbuf.rb b/lib/yahns/wbuf.rb
index 21bccce..991557e 100644
--- a/lib/yahns/wbuf.rb
+++ b/lib/yahns/wbuf.rb
@@ -30,16 +30,9 @@ require_relative 'wbuf_common'
 class Yahns::Wbuf # :nodoc:
   include Yahns::WbufCommon
 
-  # TODO: Figure out why this hack is needed to pass output buffering tests.
-  # It could be a bug in our code, Ruby, the sendfile gem, or FreeBSD itself.
-  # Tested on FreeBSD fbsd 9.2-RELEASE FreeBSD 9.2-RELEASE #0 r255898
-  # We are able to use bypass mode on Linux to reduce buffering in some
-  # cases.  Without bypass mode, we must always finish writing the entire
-  # response completely before sending more data to the client.
-  bypass_ok = RUBY_PLATFORM =~ /linux/
-
   def initialize(body, persist, tmpdir)
     @tmpio = Yahns::TmpIO.new(tmpdir)
+    @tmpdir = tmpdir
     @sf_offset = @sf_count = 0
     @wbuf_persist = persist # whether or not we keep the connection alive
     @body = body
@@ -71,16 +64,12 @@ class Yahns::Wbuf # :nodoc:
 
     # we're all caught up, try to prevent dirty data from getting flushed
     # to disk if we can help it.
-    @tmpio.truncate(@sf_offset = 0)
-    @tmpio.rewind
+    @tmpio.close
+    @sf_offset = 0
+    @tmpio = Yahns::TmpIO.new(@tmpdir)
     @bypass = true
     nil
-  end if bypass_ok
-
-  def wbuf_write(client, buf)
-    @sf_count += @tmpio.write(buf)
-    :wait_writable
-  end unless bypass_ok
+  end
 
   # called by last wbuf_flush
   def wbuf_close(client)
-- 
EW

Re: [PATCH 2/2] wbuf: avoid corrupted large responses with zero-copy sendfile

[Eric Wong]

Eric Wong <e@80x24.org> wrote:
> This bug is noticeable on a amd64 FreeBSD 9.2 VM, and possible under
> Linux, too.  This happens as a zero-copy sendfile implementation means
> pages queued for transmission by the sendfile system call should not be
> modified at any point after the sendfile syscall is made.

Btw, this gotcha is now noted in the Linux sendfile(2) manpage:

http://git.kernel.org/cgit/docs/man-pages/man-pages.git/commit/man2/sendfile.2?id=7b6a3299776b5c1c4f169a591434a855d50c68b4