socketry / async

An awesome asynchronous event-driven reactor for Ruby.
MIT License
2.09k stars 86 forks source link

Better support for `Fiber.set_scheduler`. #194

Closed ioquatix closed 1 year ago

ioquatix commented 1 year ago

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

Contribution

bruno- commented 1 year ago

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 commented 1 year ago

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 https://github.com/ruby/ruby/commit/cb8434563d3cc8fc5c3a5aa1e34ad7a9bb542cdb#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- commented 1 year ago

Thank you for the elaborate explanation