workaround reopen atomicity issues for stdio vs non-stdio
authorEric Wong <e@80x24.org>
Sun, 20 Oct 2013 04:29:55 +0000 (20 04:29 +0000)
committerEric Wong <normalperson@yhbt.net>
Sun, 20 Oct 2013 04:32:04 +0000 (20 04:32 +0000)
In multithreaded apps, we must use dup2/dup3 with a temporary
descriptor to reopen log files atomically.  This is the only way
to protect all concurrent userspace access to a file when reopening.

ref: http://bugs.ruby-lang.org/issues/9036
ref: yahns commit bcb10abe53cfb1d6a8ef7daef59eb10ced397c8a

lib/unicorn/util.rb

index f84241c..94c4e37 100644 (file)
@@ -39,7 +39,7 @@ module Unicorn::Util
     to_reopen.each do |fp|
       orig_st = begin
         fp.stat
-      rescue IOError, Errno::EBADF
+      rescue IOError, Errno::EBADF # race
         next
       end
 
@@ -50,8 +50,28 @@ module Unicorn::Util
       end
 
       begin
-        File.open(fp.path, 'a') { |tmpfp| fp.reopen(tmpfp) }
+        # stdin, stdout, stderr are special.  The following dance should
+        # guarantee there is no window where `fp' is unwritable in MRI
+        # (or any correct Ruby implementation).
+        #
+        # Fwiw, GVL has zero bearing here.  This is tricky because of
+        # the unavoidable existence of stdio FILE * pointers for
+        # std{in,out,err} in all programs which may use the standard C library
+        if fp.fileno <= 2
+          # We do not want to hit fclose(3)->dup(2) window for std{in,out,err}
+          # MRI will use freopen(3) here internally on std{in,out,err}
+          fp.reopen(fp.path, "a")
+        else
+          # We should not need this workaround, Ruby can be fixed:
+          #    http://bugs.ruby-lang.org/issues/9036
+          # MRI will not call call fclose(3) or freopen(3) here
+          # since there's no associated std{in,out,err} FILE * pointer
+          # This should atomically use dup3(2) (or dup2(2)) syscall
+          File.open(fp.path, "a") { |tmpfp| fp.reopen(tmpfp) }
+        end
+
         fp.sync = true
+        fp.flush # IO#sync=true may not implicitly flush
         new_st = fp.stat
 
         # this should only happen in the master: