Skip to content

Commit

Permalink
Send :fiber_switch event for almost every fiber_switch (#4207)
Browse files Browse the repository at this point in the history 
With this patch, TracePoint receives a `:fiber_switch` event for
_almost_ every fiber switch.  Previously, it would not be sent when an
exception was going to be raised. Now the event should only be blockable
by an interrupt (including `Thread#raise`) or a fatal error.

Additionally, interrupts will now be checked on the return fiber
_before_ re-raising the terminating unhandled exception.  And a fiber
that terminates with an unhandled exception no longer creates a pending
interrupt on its thread.  The exception will be raised in the return
fiber the same way as `Fiber#raise`: using `cont.value` with `cont.argc
== -1`

I moved `rb_exc_raise` from `fiber_store` to the end of `fiber_switch`
after _all_ of the other cleanup code: `fiber_stack_release`,
`th->blocking` increment, `RUBY_VM_CHECK_INTS`, and `EXEC_EVENT_HOOK`.
It seems to me that skipping those other cleanup steps may have also
resulted in other bugs.
  • Loading branch information
@nevans
nevans committed Feb 21, 2021
1 parent aeac4dd commit 3ee4fa9
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 18 deletions.
36 changes: 18 additions & 18 deletions cont.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2005,7 +2005,7 @@ rb_fiber_set_scheduler(VALUE klass, VALUE scheduler)
return rb_fiber_scheduler_set(scheduler);
}

static void rb_fiber_terminate(rb_fiber_t *fiber, int need_interrupt);
static void rb_fiber_terminate(rb_fiber_t *fiber, int need_interrupt, VALUE err);

void
rb_fiber_start(void)
Expand All @@ -2015,6 +2015,7 @@ rb_fiber_start(void)
rb_proc_t *proc;
enum ruby_tag_type state;
int need_interrupt = TRUE;
VALUE err = Qfalse;

VM_ASSERT(th->ec == GET_EC());
VM_ASSERT(FIBER_RESUMED_P(fiber));
Expand All @@ -2041,22 +2042,21 @@ rb_fiber_start(void)
EC_POP_TAG();

if (state) {
VALUE err = th->ec->errinfo;
err = th->ec->errinfo;
VM_ASSERT(FIBER_RESUMED_P(fiber));

if (state == TAG_RAISE || state == TAG_FATAL) {
if (state == TAG_RAISE) {
// noop...
} else if (state == TAG_FATAL) {
rb_threadptr_pending_interrupt_enque(th, err);
}
else {
err = rb_vm_make_jump_tag_but_local_jump(state, err);
if (!NIL_P(err)) {
rb_threadptr_pending_interrupt_enque(th, err);
}
}
need_interrupt = TRUE;
}

rb_fiber_terminate(fiber, need_interrupt);
rb_fiber_terminate(fiber, need_interrupt, err);
VM_UNREACHABLE(rb_fiber_start);
}

Expand Down Expand Up @@ -2182,7 +2182,7 @@ rb_fiber_current(void)
}

// Prepare to execute next_fiber on the given thread.
static inline VALUE
static inline void
fiber_store(rb_fiber_t *next_fiber, rb_thread_t *th)
{
rb_fiber_t *fiber;
Expand All @@ -2206,13 +2206,6 @@ fiber_store(rb_fiber_t *next_fiber, rb_thread_t *th)

fiber_status_set(next_fiber, FIBER_RESUMED);
fiber_setcontext(next_fiber, fiber);

fiber = th->ec->fiber_ptr;

/* Raise an exception if that was the result of executing the fiber */
if (fiber->cont.argc == -1) rb_exc_raise(fiber->cont.value);

return fiber->cont.value;
}

static inline VALUE
Expand Down Expand Up @@ -2285,7 +2278,7 @@ fiber_switch(rb_fiber_t *fiber, int argc, const VALUE *argv, int kw_splat, VALUE
cont->kw_splat = kw_splat;
cont->value = make_passing_arg(argc, argv);

value = fiber_store(fiber, th);
fiber_store(fiber, th);

if (RTEST(resuming_fiber) && FIBER_TERMINATED_P(fiber)) {
fiber_stack_release(fiber);
Expand All @@ -2299,6 +2292,9 @@ fiber_switch(rb_fiber_t *fiber, int argc, const VALUE *argv, int kw_splat, VALUE

EXEC_EVENT_HOOK(th->ec, RUBY_EVENT_FIBER_SWITCH, th->self, 0, 0, 0, Qnil);

current_fiber = th->ec->fiber_ptr;
value = current_fiber->cont.value;
if (current_fiber->cont.argc == -1) rb_exc_raise(value);
return value;
}

Expand Down Expand Up @@ -2365,7 +2361,7 @@ rb_fiber_close(rb_fiber_t *fiber)
}

static void
rb_fiber_terminate(rb_fiber_t *fiber, int need_interrupt)
rb_fiber_terminate(rb_fiber_t *fiber, int need_interrupt, VALUE err)
{
VALUE value = fiber->cont.value;
rb_fiber_t *next_fiber;
Expand All @@ -2380,7 +2376,11 @@ rb_fiber_terminate(rb_fiber_t *fiber, int need_interrupt)

next_fiber = return_fiber(true);
if (need_interrupt) RUBY_VM_SET_INTERRUPT(&next_fiber->cont.saved_ec);
fiber_switch(next_fiber, 1, &value, RB_NO_KEYWORDS, Qfalse, false);
if (RTEST(err))
fiber_switch(next_fiber, -1, &err, RB_NO_KEYWORDS, Qfalse, false);
else
fiber_switch(next_fiber, 1, &value, RB_NO_KEYWORDS, Qfalse, false);
VM_UNREACHABLE(rb_fiber_terminate);
}

VALUE
Expand Down
62 changes: 62 additions & 0 deletions test/ruby/test_settracefunc.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1593,6 +1593,33 @@ def test_fiber_switch
assert_equal ev, :fiber_switch
}

# test for raise into resumable fiber
evs = []
f = nil
TracePoint.new(:raise, :fiber_switch){|tp|
next unless target_thread?
evs << [tp.event, Fiber.current]
}.enable{
f = Fiber.new{
Fiber.yield # will raise
Fiber.yield # unreachable
}
begin
f.resume
f.raise StopIteration
rescue StopIteration
evs << :rescued
end
}
assert_equal [:fiber_switch, f], evs[0], "initial resume"
assert_equal [:fiber_switch, Fiber.current], evs[1], "Fiber.yield"
assert_equal [:fiber_switch, f], evs[2], "fiber.raise"
assert_equal [:raise, f], evs[3], "fiber.raise"
assert_equal [:fiber_switch, Fiber.current], evs[4], "terminated with raise"
assert_equal [:raise, Fiber.current], evs[5], "terminated with raise"
assert_equal :rescued, evs[6]
assert_equal 7, evs.size

# test for transfer
evs = []
TracePoint.new(:fiber_switch){|tp|
Expand All @@ -1615,6 +1642,41 @@ def test_fiber_switch
evs.each{|ev|
assert_equal ev, :fiber_switch
}

# test for raise and from transferring fibers
evs = []
f1 = f2 = nil
TracePoint.new(:raise, :fiber_switch){|tp|
next unless target_thread?
evs << [tp.event, Fiber.current]
}.enable{
f1 = Fiber.new{
f2.transfer
f2.raise ScriptError
Fiber.yield :ok
}
f2 = Fiber.new{
f1.transfer
f1.transfer
}
begin
f1.resume
rescue ScriptError
evs << :rescued
end
}
assert_equal [:fiber_switch, f1], evs[0], "initial resume"
assert_equal [:fiber_switch, f2], evs[1], "f2.transfer"
assert_equal [:fiber_switch, f1], evs[2], "f1.transfer"
assert_equal [:fiber_switch, f2], evs[3], "f2.raise ScriptError"
assert_equal [:raise, f2], evs[4], "f2.raise ScriptError"
assert_equal [:fiber_switch, f1], evs[5], "f2 unhandled exception"
assert_equal [:raise, f1], evs[6], "f2 unhandled exception"
assert_equal [:fiber_switch, Fiber.current], evs[7], "f1 unhandled exception"
assert_equal [:raise, Fiber.current], evs[8], "f1 unhandled exception"
assert_equal :rescued, evs[9], "rescued everything"
assert_equal 10, evs.size

end

def test_tracepoint_callee_id
Expand Down

0 comments on commit 3ee4fa9

Please sign in to comment.