Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let Fiber#raise work with transferring fibers (implicit) #3795

Merged
merged 1 commit into from Dec 11, 2020

Conversation

nevans
Copy link
Contributor

@nevans nevans commented Nov 20, 2020

This automatically chooses whether to use transfer on a transferring
fiber or resume on a yielding fiber. If the fiber is resuming, it
raises a FiberError.

This is an alternative approach to #3783. I personally prefer the explicit approach, but I'll let others decide. 馃檪

Ruby Issue: https://bugs.ruby-lang.org/issues/17331

This automatically choosess whether to use transfer on a transferring
fiber or resume on a yielding fiber.  If the fiber is resuming, it
raises a FiberError.
@nevans nevans force-pushed the fiber-raise-with-transferring-implicit branch from 335c67a to 1cf9de0 Compare November 20, 2020 14:31
@eregon
Copy link
Member

eregon commented Nov 20, 2020

This looks to me, 馃憤

@eregon eregon requested a review from ko1 November 20, 2020 19:01
cont.c Show resolved Hide resolved
@ioquatix
Copy link
Member

I will try to review this code next weekend. We should definitely get this into Ruby 3 if possible.

@nevans
Copy link
Contributor Author

nevans commented Dec 7, 2020

@ioquatix @eregon If the following makes sense, I can make new tickets and PRs:

The questions of "how and when should control pass back to the current fiber?" and "exceptions raised from terminating transferred fibers will return to the root fiber chain" keep coming up for both this and for #3766, and they are both reasonable questions. Rather than deal with that complexity, I've been punting: that's out of scope for this; just call your scheduler library and let it coordinate, right?

But I think with a couple of optional hooks on the scheduler, we can answer both of those questions. Just as a quick brainstorm, what do you think about something like the following?

To answer: "how and when should control pass back to the current fiber?"

# Called before transferring into another fiber.
# @param target [Fiber] the fiber that will be transferred into
# @param reason [:transfer, :raise, :cancel] How the transfer was initiated 
# @param args the arguments sent to transfer, raise, or cancel.
#
# @raise [FiberTransferInterrupted] to prevent the transfer without raising an
#     exception in the calling fiber.
# @raise [Exception] prevents the transfer and raises in the calling fiber
#
# This can be used to ensure the current fiber is appropriately scheduled for
# return, and it can also prevent the transfer or schedule the transfer to
# happen asynchronously.
#
# In addition to raising exceptions, any call to a fiber switching method (e.g.
# resume, yield, or transfer) will prevent the transfer. When a transfer is
# prevented, any associated cancellation or exception will not happen.
#
# This will only be called for transfers, not for resume, yield, or termination.
def transferring(target, reason, *args)
  if Fiber.current == @scheduler
    # always allow transfer from scheduler
  elsif target == @scheduler
    # guard transfer to the scheduler
    raise FiberError, "invalid transfer to scheduler" if invalid?(reason, *args)
  else
    # schedule all transfers instead of running immediately
    @next_tick << [target, reason, *args]
    @next_tick << [Fiber.current, :transfer] unless blocking?
    @scheduler.transfer
  end
end

This would be useful for more than just #raise and #cancel. This way, if non-scheduler code calls Fiber#transfer (or indirectly transfers via #raise or #cancel) it doesn't need to confuse or break the scheduler. It allows the scheduler some control over transfer and over how raise/cancel will work with transferring fibers.

And to handle: "exceptions raised from terminating transferred fibers will return to the root fiber chain"

# Select the return fiber for a transferring fiber when it terminates.
# @param terminating [Fiber] The terminating fiber
# @param retval [Object] return value of the terminating fiber
# @param error [Exception | nil] raised by terminating fiber
# @return [Fiber | nil] fiber to transfer into. `nil` uses default behavior
#
# If the selected return fiber can't be transferred to (because it is yielding
# or resuming or dead), FiberError will be raised on root fiber chain.
#
# This will be run in the terminating fiber after its block has completed.
# If this raises an exception, that will be raised on the root fiber chain.
#
def select_return_fiber(terminating, retval, error)
  supervisor = @supervised[terminating]
  if supervisor&.alive?
    supervisor
  elsif @scheduled.include?(terminating)
    @scheduler
  elsif !error
    raise FiberError, "unsupervised transfer fiber terminated"
  end
end

In addition to answering the question raised by this PR and by #3766, I think this also simplifies some other useful patterns.

It would also let me easily fix one of the things that ruby 3.0 breaks in my current code: I like to "init" my transfer fibers by first resuming into them from their supervisor and then immediately transferring back out. That sets the return_fiber for when it terminates. A workaround is to use an ensured supervisor.transfer on the last line and then abandon the almost dead fiber. But that might lead to a bug later if some other code held onto a reference to that fiber, saw it was still alive, and transferred into it. (unlikely, but plausible). And it's still brittle: if any errant code calls Fiber.yield, the return_fiber will be lost and can never be recovered. Letting the scheduler manage this would provide the lost ruby 2 functionality and more.

What do you think?

@ioquatix
Copy link
Member

ioquatix commented Dec 8, 2020

My initial reaction is, it's too complicated, but I will take a look this weekend. I ran out of time the past weekend.

@nevans
Copy link
Contributor Author

nevans commented Dec 8, 2020

My initial reaction is, it's too complicated, but I will take a look this weekend. I ran out of time the past weekend.

Yeah, I'm not super happy with either of them, but especially transferring callback. But I really really really want some ability to delegate select_return_fiber to the scheduler, because that simplifies the supervisor pattern. Another approach might be to attach a handler to each fiber. But (IMO) the scheduler should be able to monitor and regulate all transferring fibers that live inside its thread. The per-fiber approach doesn't allow that.

@ioquatix
Copy link
Member

I have reviewed this PR and I am happy with it.

@ioquatix ioquatix merged commit 31e8de2 into ruby:master Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants