yahns Ruby server user/dev discussion
 help / color / mirror / code / Atom feed
* [PATCH 0/2] FreeBSD-related fixes (affects Linux users, too)
@ 2014-07-16 19:57 Eric Wong
  2014-07-16 19:57 ` [PATCH 1/2] test_server: avoid multiple workers for dead parent check Eric Wong
  2014-07-16 19:57 ` [PATCH 2/2] wbuf: avoid corrupted large responses with zero-copy sendfile Eric Wong
  0 siblings, 2 replies; 4+ messages in thread
From: Eric Wong @ 2014-07-16 19:57 UTC (permalink / raw)
  To: yahns-public; +Cc: e

[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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 1/2] test_server: avoid multiple workers for dead parent check
  2014-07-16 19:57 [PATCH 0/2] FreeBSD-related fixes (affects Linux users, too) Eric Wong
@ 2014-07-16 19:57 ` Eric Wong
  2014-07-16 19:57 ` [PATCH 2/2] wbuf: avoid corrupted large responses with zero-copy sendfile Eric Wong
  1 sibling, 0 replies; 4+ messages in thread
From: Eric Wong @ 2014-07-16 19:57 UTC (permalink / raw)
  To: yahns-public; +Cc: e, EW

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 2/2] wbuf: avoid corrupted large responses with zero-copy sendfile
  2014-07-16 19:57 [PATCH 0/2] FreeBSD-related fixes (affects Linux users, too) Eric Wong
  2014-07-16 19:57 ` [PATCH 1/2] test_server: avoid multiple workers for dead parent check Eric Wong
@ 2014-07-16 19:57 ` Eric Wong
  2015-03-07 21:36   ` Eric Wong
  1 sibling, 1 reply; 4+ messages in thread
From: Eric Wong @ 2014-07-16 19:57 UTC (permalink / raw)
  To: yahns-public; +Cc: e, 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/2] wbuf: avoid corrupted large responses with zero-copy sendfile
  2014-07-16 19:57 ` [PATCH 2/2] wbuf: avoid corrupted large responses with zero-copy sendfile Eric Wong
@ 2015-03-07 21:36   ` Eric Wong
  0 siblings, 0 replies; 4+ messages in thread
From: Eric Wong @ 2015-03-07 21:36 UTC (permalink / raw)
  To: yahns-public

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-03-07 21:36 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-16 19:57 [PATCH 0/2] FreeBSD-related fixes (affects Linux users, too) Eric Wong
2014-07-16 19:57 ` [PATCH 1/2] test_server: avoid multiple workers for dead parent check Eric Wong
2014-07-16 19:57 ` [PATCH 2/2] wbuf: avoid corrupted large responses with zero-copy sendfile Eric Wong
2015-03-07 21:36   ` Eric Wong

Code repositories for project(s) associated with this inbox:

	../../../yahns.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).