normal	2018-09-13 05:49:24 +0900 (Thu, 13 Sep 2018)

  New Revision: 64706

  https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=64706

  Log:
    fiber: fix crash on GC after forking
    
    Remove the remainder of ROOT_FIBER_CONTEXT use and unnecessary
    differences between the root and non-root fiber.  This makes
    it easier to follow new root fiber at fork time.
    
    Multiple sources of truth often leads to bugs, as in this case.
    We can determinte root fiber by checking a fiber against the root_fiber
    of its owner thread.  The new `fiber_is_root_p' function
    supports that.
    
    Now, we can care only about free-ing/recycling/munmap-ing stacks
    as appropriate.
    
    [Bug #15050]

  Modified files:
    trunk/cont.c
    trunk/test/ruby/test_fiber.rb
Index: cont.c
===================================================================
--- cont.c	(revision 64705)
+++ cont.c	(revision 64706)
@@ -77,8 +77,7 @@ static long pagesize; https://github.com/ruby/ruby/blob/trunk/cont.c#L77
 
 enum context_type {
     CONTINUATION_CONTEXT = 0,
-    FIBER_CONTEXT = 1,
-    ROOT_FIBER_CONTEXT = 2
+    FIBER_CONTEXT = 1
 };
 
 struct cont_saved_vm_stack {
@@ -360,6 +359,12 @@ cont_mark(void *ptr) https://github.com/ruby/ruby/blob/trunk/cont.c#L359
     RUBY_MARK_LEAVE("cont");
 }
 
+static int
+fiber_is_root_p(const rb_fiber_t *fib)
+{
+    return fib == fib->cont.saved_ec.thread_ptr->root_fiber;
+}
+
 static void
 cont_free(void *ptr)
 {
@@ -378,24 +383,17 @@ cont_free(void *ptr) https://github.com/ruby/ruby/blob/trunk/cont.c#L383
 	/* fiber */
 	const rb_fiber_t *fib = (rb_fiber_t*)cont;
 #ifdef _WIN32
-	if (cont->type != ROOT_FIBER_CONTEXT) {
+        if (!fiber_is_root_p(fib)) {
 	    /* don't delete root fiber handle */
 	    if (fib->fib_handle) {
 		DeleteFiber(fib->fib_handle);
 	    }
 	}
 #else /* not WIN32 */
+        /* fib->ss_sp == NULL is possible for root fiber */
 	if (fib->ss_sp != NULL) {
-	    if (cont->type == ROOT_FIBER_CONTEXT) {
-		rb_bug("Illegal root fiber parameter");
-	    }
 	    munmap((void*)fib->ss_sp, fib->ss_size);
 	}
-	else {
-	    /* It may reached here when finalize */
-	    /* TODO examine whether it is a bug */
-	    /* rb_bug("cont_free: release self"); */
-	}
 #endif
     }
 #else /* not FIBER_USE_NATIVE */
@@ -1526,7 +1524,7 @@ root_fiber_alloc(rb_thread_t *th) https://github.com/ruby/ruby/blob/trunk/cont.c#L1524
     rb_fiber_t *fib = th->ec->fiber_ptr;
 
     VM_ASSERT(DATA_PTR(fibval) == NULL);
-    VM_ASSERT(fib->cont.type == ROOT_FIBER_CONTEXT);
+    VM_ASSERT(fib->cont.type == FIBER_CONTEXT);
     VM_ASSERT(fib->status == FIBER_RESUMED);
 
     th->root_fiber = fib;
@@ -1555,7 +1553,7 @@ rb_threadptr_root_fiber_setup(rb_thread_ https://github.com/ruby/ruby/blob/trunk/cont.c#L1553
 {
     rb_fiber_t *fib = ruby_mimmalloc(sizeof(rb_fiber_t));
     MEMZERO(fib, rb_fiber_t, 1);
-    fib->cont.type = ROOT_FIBER_CONTEXT;
+    fib->cont.type = FIBER_CONTEXT;
     fib->cont.saved_ec.fiber_ptr = fib;
     fib->cont.saved_ec.thread_ptr = th;
     fiber_status_set(fib, FIBER_RESUMED); /* skip CREATED */
@@ -1571,7 +1569,7 @@ rb_threadptr_root_fiber_release(rb_threa https://github.com/ruby/ruby/blob/trunk/cont.c#L1569
 	/* ignore. A root fiber object will free th->ec */
     }
     else {
-	VM_ASSERT(th->ec->fiber_ptr->cont.type == ROOT_FIBER_CONTEXT);
+	VM_ASSERT(th->ec->fiber_ptr->cont.type == FIBER_CONTEXT);
 	VM_ASSERT(th->ec->fiber_ptr->cont.self == 0);
 	fiber_free(th->ec->fiber_ptr);
 
@@ -1818,7 +1816,7 @@ rb_fiber_resume(VALUE fibval, int argc, https://github.com/ruby/ruby/blob/trunk/cont.c#L1816
 {
     rb_fiber_t *fib = fiber_ptr(fibval);
 
-    if (fib->prev != 0 || fib->cont.type == ROOT_FIBER_CONTEXT) {
+    if (fib->prev != 0 || fiber_is_root_p(fib)) {
 	rb_raise(rb_eFiberError, "double resume");
     }
     if (fib->transferred != 0) {
@@ -1997,7 +1995,6 @@ rb_fiber_atfork(rb_thread_t *th) https://github.com/ruby/ruby/blob/trunk/cont.c#L1995
     if (th->root_fiber) {
         if (&th->root_fiber->cont.saved_ec != th->ec) {
             th->root_fiber = th->ec->fiber_ptr;
-            th->root_fiber->cont.type = ROOT_FIBER_CONTEXT;
         }
         th->root_fiber->prev = 0;
     }
Index: test/ruby/test_fiber.rb
===================================================================
--- test/ruby/test_fiber.rb	(revision 64705)
+++ test/ruby/test_fiber.rb	(revision 64706)
@@ -260,18 +260,25 @@ class TestFiber < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_fiber.rb#L260
   end
 
   def test_fork_from_fiber
-    begin
-      pid = Process.fork{}
-    rescue NotImplementedError
-      return
-    else
-      Process.wait(pid)
-    end
+    skip 'fork not supported' unless Process.respond_to?(:fork)
+    pid = nil
     bug5700 = '[ruby-core:41456]'
     assert_nothing_raised(bug5700) do
       Fiber.new do
         pid = fork do
-          Fiber.new {}.transfer
+          xpid = nil
+          Fiber.new {
+            xpid = fork do
+              # enough to trigger GC on old root fiber
+              10000.times do
+                Fiber.new {}.transfer
+                Fiber.new { Fiber.yield }
+              end
+              exit!(0)
+            end
+          }.transfer
+          _, status = Process.waitpid2(xpid)
+          exit!(status.success?)
         end
       end.resume
     end

--
ML: ruby-changes@q...
Info: http://www.atdot.net/~ko1/quickml/