[前][次][番号順一覧][スレッド一覧]

ruby-changes:63333

From: Koichi <ko1@a...>
Date: Mon, 12 Oct 2020 22:59:02 +0900 (JST)
Subject: [ruby-changes:63333] bf3b2a4374 (master): relax Fiber#transfer's restriction

https://git.ruby-lang.org/ruby.git/commit/?id=bf3b2a4374

From bf3b2a43741e4f72be21bc6acf24d37e7fcff61c Mon Sep 17 00:00:00 2001
From: Koichi Sasada <ko1@a...>
Date: Thu, 8 Oct 2020 01:15:32 +0900
Subject: relax Fiber#transfer's restriction

Using Fiber#transfer with Fiber#resume for a same Fiber is
limited (once Fiber#transfer is called for a fiber, the fiber
can not be resumed more). This restriction was introduced to
protect the resume/yield chain, but we realized that it is too much
to protect the chain. Instead of the current restriction, we
introduce some other protections.

(1) can not transfer to the resuming fiber.
(2) can not transfer to the yielding fiber.
(3) can not resume transferred fiber.
(4) can not yield from not-resumed fiber.

[Bug #17221]

Also at the end of a transferred fiber, it had continued on root fiber.
However, if the root fiber resumed a fiber (and that fiber can resumed
another fiber), this behavior also breaks the resume/yield chain.
So at the end of a transferred fiber, switch to the edge of resume
chain from root fiber.
For example, root fiber resumed f1 and f1 resumed f2, transferred to
f3 and f3 terminated, then continue from the fiber f2 (it was continued
from root fiber without this patch).

diff --git a/cont.c b/cont.c
index 561398d..97689ab 100644
--- a/cont.c
+++ b/cont.c
@@ -208,6 +208,7 @@ typedef struct rb_context_struct { https://github.com/ruby/ruby/blob/trunk/cont.c#L208
 
 /*
  * Fiber status:
+ * Fiber status:
  *    [Fiber.new] ------> FIBER_CREATED
  *                        | [Fiber#resume]
  *                        v
@@ -235,14 +236,11 @@ struct rb_fiber_struct { https://github.com/ruby/ruby/blob/trunk/cont.c#L236
     rb_context_t cont;
     VALUE first_proc;
     struct rb_fiber_struct *prev;
-    BITFIELD(enum fiber_status, status, 2);
-    /* If a fiber invokes by "transfer",
-     * then this fiber can't be invoked by "resume" any more after that.
-     * You shouldn't mix "transfer" and "resume".
-     */
-    unsigned int transferred : 1;
+    VALUE resuming_fiber;
 
+    BITFIELD(enum fiber_status, status, 2);
     /* Whether the fiber is allowed to implicitly yield. */
+    unsigned int yielding : 1;
     unsigned int blocking : 1;
 
     struct coroutine_context context;
@@ -919,11 +917,13 @@ cont_mark(void *ptr) https://github.com/ruby/ruby/blob/trunk/cont.c#L917
     RUBY_MARK_LEAVE("cont");
 }
 
+#if 0
 static int
 fiber_is_root_p(const rb_fiber_t *fiber)
 {
     return fiber == fiber->cont.saved_ec.thread_ptr->root_fiber;
 }
+#endif
 
 static void
 cont_free(void *ptr)
@@ -2010,25 +2010,33 @@ fiber_current(void) https://github.com/ruby/ruby/blob/trunk/cont.c#L2010
 }
 
 static inline rb_fiber_t*
-return_fiber(void)
+return_fiber(bool terminate)
 {
     rb_fiber_t *fiber = fiber_current();
     rb_fiber_t *prev = fiber->prev;
 
-    if (!prev) {
+    if (prev) {
+        fiber->prev = NULL;
+        prev->resuming_fiber = Qnil;
+        return prev;
+    }
+    else {
+        if (!terminate) {
+            rb_raise(rb_eFiberError, "attempt to yield on a not resumed fiber");
+        }
+
         rb_thread_t *th = GET_THREAD();
         rb_fiber_t *root_fiber = th->root_fiber;
 
         VM_ASSERT(root_fiber != NULL);
 
-        if (root_fiber == fiber) {
-            rb_raise(rb_eFiberError, "can't yield from root fiber");
+        // search resuming fiber
+        for (fiber = root_fiber;
+             RTEST(fiber->resuming_fiber);
+             fiber = fiber_ptr(fiber->resuming_fiber)) {
         }
-        return root_fiber;
-    }
-    else {
-        fiber->prev = NULL;
-        return prev;
+
+        return fiber;
     }
 }
 
@@ -2073,7 +2081,7 @@ fiber_store(rb_fiber_t *next_fiber, rb_thread_t *th) https://github.com/ruby/ruby/blob/trunk/cont.c#L2081
 }
 
 static inline VALUE
-fiber_switch(rb_fiber_t *fiber, int argc, const VALUE *argv, int is_resume, int kw_splat)
+fiber_switch(rb_fiber_t *fiber, int argc, const VALUE *argv, int kw_splat, VALUE resuming_fiber, bool yielding)
 {
     VALUE value;
     rb_context_t *cont = &fiber->cont;
@@ -2120,11 +2128,21 @@ fiber_switch(rb_fiber_t *fiber, int argc, const VALUE *argv, int is_resume, int https://github.com/ruby/ruby/blob/trunk/cont.c#L2128
 
     VM_ASSERT(FIBER_RUNNABLE_P(fiber));
 
-    if (is_resume) {
+    rb_fiber_t *current_fiber = fiber_current();
+
+    VM_ASSERT(!RTEST(current_fiber->resuming_fiber));
+    if (RTEST(resuming_fiber)) {
+        current_fiber->resuming_fiber = resuming_fiber;
         fiber->prev = fiber_current();
+        fiber->yielding = 0;
     }
 
-    if (fiber_current()->blocking) {
+    VM_ASSERT(!current_fiber->yielding);
+    if (yielding) {
+        current_fiber->yielding = 1;
+    }
+
+    if (current_fiber->blocking) {
         th->blocking -= 1;
     }
 
@@ -2134,7 +2152,7 @@ fiber_switch(rb_fiber_t *fiber, int argc, const VALUE *argv, int is_resume, int https://github.com/ruby/ruby/blob/trunk/cont.c#L2152
 
     value = fiber_store(fiber, th);
 
-    if (is_resume && FIBER_TERMINATED_P(fiber)) {
+    if (RTEST(resuming_fiber) && FIBER_TERMINATED_P(fiber)) {
         fiber_stack_release(fiber);
     }
 
@@ -2152,7 +2170,7 @@ fiber_switch(rb_fiber_t *fiber, int argc, const VALUE *argv, int is_resume, int https://github.com/ruby/ruby/blob/trunk/cont.c#L2170
 VALUE
 rb_fiber_transfer(VALUE fiber_value, int argc, const VALUE *argv)
 {
-    return fiber_switch(fiber_ptr(fiber_value), argc, argv, 0, RB_NO_KEYWORDS);
+    return fiber_switch(fiber_ptr(fiber_value), argc, argv, RB_NO_KEYWORDS, Qfalse, false);
 }
 
 VALUE
@@ -2181,29 +2199,38 @@ rb_fiber_terminate(rb_fiber_t *fiber, int need_interrupt) https://github.com/ruby/ruby/blob/trunk/cont.c#L2199
     fiber->cont.machine.stack = NULL;
     fiber->cont.machine.stack_size = 0;
 
-    next_fiber = return_fiber();
+    next_fiber = return_fiber(true);
     if (need_interrupt) RUBY_VM_SET_INTERRUPT(&next_fiber->cont.saved_ec);
-    fiber_switch(next_fiber, 1, &value, 0, RB_NO_KEYWORDS);
+    fiber_switch(next_fiber, 1, &value, RB_NO_KEYWORDS, Qfalse, false);
 }
 
 VALUE
 rb_fiber_resume_kw(VALUE fiber_value, int argc, const VALUE *argv, int kw_splat)
 {
     rb_fiber_t *fiber = fiber_ptr(fiber_value);
+    rb_fiber_t *current_fiber = fiber_current();
 
     if (argc == -1 && FIBER_CREATED_P(fiber)) {
         rb_raise(rb_eFiberError, "cannot raise exception on unborn fiber");
     }
-
-    if (fiber->prev != 0 || fiber_is_root_p(fiber)) {
-        rb_raise(rb_eFiberError, "double resume");
+    else if (FIBER_TERMINATED_P(fiber)) {
+        rb_raise(rb_eFiberError, "attempt to resume a terminated fiber");
     }
-
-    if (fiber->transferred != 0) {
-        rb_raise(rb_eFiberError, "cannot resume transferred Fiber");
+    else if (fiber == current_fiber) {
+        rb_raise(rb_eFiberError, "attempt to resume the current fiber");
+    }
+    else if (fiber->prev != NULL) {
+        rb_raise(rb_eFiberError, "attempt to resume a resumed fiber (double resume)");
+    }
+    else if (RTEST(fiber->resuming_fiber)) {
+        rb_raise(rb_eFiberError, "attempt to resume a resuming fiber");
+    }
+    else if (fiber->prev == NULL &&
+             (!fiber->yielding && fiber->status != FIBER_CREATED)) {
+        rb_raise(rb_eFiberError, "attempt to resume a transferring fiber");
     }
 
-    return fiber_switch(fiber, argc, argv, 1, kw_splat);
+    return fiber_switch(fiber, argc, argv, kw_splat, fiber_value, false);
 }
 
 VALUE
@@ -2215,13 +2242,13 @@ rb_fiber_resume(VALUE fiber_value, int argc, const VALUE *argv) https://github.com/ruby/ruby/blob/trunk/cont.c#L2242
 VALUE
 rb_fiber_yield_kw(int argc, const VALUE *argv, int kw_splat)
 {
-    return fiber_switch(return_fiber(), argc, argv, 0, kw_splat);
+    return fiber_switch(return_fiber(false), argc, argv, kw_splat, Qfalse, true);
 }
 
 VALUE
 rb_fiber_yield(int argc, const VALUE *argv)
 {
-    return fiber_switch(return_fiber(), argc, argv, 0, RB_NO_KEYWORDS);
+    return fiber_switch(return_fiber(false), argc, argv, RB_NO_KEYWORDS, Qfalse, true);
 }
 
 void
@@ -2362,8 +2389,13 @@ static VALUE https://github.com/ruby/ruby/blob/trunk/cont.c#L2389
 rb_fiber_m_transfer(int argc, VALUE *argv, VALUE fiber_value)
 {
     rb_fiber_t *fiber = fiber_ptr(fiber_value);
-    fiber->transferred = 1;
-    return fiber_switch(fiber, argc, argv, 0, rb_keyword_given_p());
+    if (RTEST(fiber->resuming_fiber)) {
+        rb_raise(rb_eFiberError, "attempt to transfer to a resuming fiber");
+    }
+    if (fiber->yielding) {
+        rb_raise(rb_eFiberError, "attempt to transfer to a yielding fiber");
+    }
+    return fiber_switch(fiber, argc, argv, rb_keyword_given_p(), Qfalse, false);
 }
 
 /*
@@ -2411,8 +2443,8 @@ fiber_to_s(VALUE fiber_value) https://github.com/ruby/ruby/blob/trunk/cont.c#L2443
     const rb_proc_t *proc;
     char status_info[0x20];
 
-    if (fiber->transferred) {
-        snprintf(status_info, 0x20, " (%s, transferred)", fiber_status_name(fiber->status));
+    if (RTEST(fiber->resuming_fiber)) {
+        snprintf(status_info, 0x20, " (%s by resuming)", fiber_status_name(fiber->status));
     }
     else {
         snprintf(status_info, 0x20, " (%s)", fiber_status_name(fiber->status));
diff --git a/spec/ruby/core/fiber/resume_spec.rb b/spec/ruby/core/fiber/resume_spec.rb
index 97495c5..273bc86 100644
--- a/spec/ruby/core/fiber/resume_spec.rb
+++ b/spec/ruby/core/fiber/resume_spec.rb
@@ -6,9 +6,40 @@ describe "Fiber#resume" do https://github.com/ruby/ruby/blob/trunk/spec/ruby/core/fiber/resume_spec.rb#L6
 end
 
 describe "Fiber#resume" do
-  it "raises a FiberError if the Fiber tries to resume itself" do
-    fiber = Fiber.new { fiber.resume }
-    -> { fiber.resume }.should raise_error(FiberError, /double resume/)
+  it "runs until Fiber.yield" do
+    obj = mock('obj')
+    obj.should_not_receive(:do)
+    fiber = Fiber.new { 1 + 2; Fiber.yield; obj.do }
+    fiber.resume
+  end
+
+  it "resumes from the last call to Fiber.yield on subsequent invocations" do
+    fiber = Fiber.new { Fiber.yield :fir (... truncated)

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

[前][次][番号順一覧][スレッド一覧]