Skip to content

Better support for Fiber.set_scheduler. #194

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 2, 2022
Merged

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Nov 2, 2022

Add support for scheduler_close hook which is canonical for Fiber.set_scheduler usage, without it, the scheduled fibers won't run unless the scheduler is explicitly invoked.

Types of Changes

  • Bug fix.
  • New feature.

Contribution

@ioquatix ioquatix merged commit 8919adf into main Nov 2, 2022
@ioquatix ioquatix deleted the fiber-set_scheduler branch November 2, 2022 03:46
@bruno-
Copy link
Contributor

bruno- commented Nov 2, 2022

Hi,

I always thought this is handled via #close method.
Were there any changes to the FiberScheduler interface for 3.2? I checked the messages of your commits to Ruby for the last couple months and I couldn't find anything...

@ioquatix
Copy link
Member Author

ioquatix commented Nov 3, 2022

When I first implemented the fiber scheduler, I called Fiber.scheduler.close when going out of scope (end of thread, process, etc).

I found that this was pretty annoying because you have to be incredibly careful if you call Fiber.set_scheduler(nil) from your close implementation (it can result in unbounded recursion). It seemed reasonable to me that if Async::Reactor#initialize can call Fiber.set_scheduler(self), Async::Reactor#close should also be able to call Fiber.set_scheduler(nil). But this was impossible with that design, because Fiber.set_scheduler(nil) would invoke the close method again...

So the hook implementation now looks like this: https://github.com/ruby/ruby/blob/59a6caf83acf32915abf11ae81ca3167d2577b21/scheduler.c#L210-L240.

See ruby/ruby@cb84345#diff-da9921e67ecf57099a9cb9f188c5f6289d7d1aeecaf9548008b98e4c716dffeaR117-R121 for how the sample scheduler implementation was changed.

Basically, when going out of scope, it will now try to invoke scheduler_close which should run the fiber scheduler and then close it. You can also expose a scheduler.close to the user, which only needs to close the reactor. This was the original behaviour of Async::Reactor and it wasn't safe to add Fiber.set_scheduler(nil) to the Async::Reactor implementation and instead I ended up having to add it to the implementation of Kernel::Async (which is fine). But ideally, calling Async::Reactor#close should now invoke Fiber.set_scheduler(nil).

@bruno-
Copy link
Contributor

bruno- commented Nov 4, 2022

Thank you for the elaborate explanation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants