Skip to content

Refactor EventLoop interface for sleeps & select timeouts#14980

Merged
straight-shoota merged 3 commits intocrystal-lang:masterfrom
ysbaddaden:refactor/select-timeout-and-sleeps
Sep 9, 2024
Merged

Refactor EventLoop interface for sleeps & select timeouts#14980
straight-shoota merged 3 commits intocrystal-lang:masterfrom
ysbaddaden:refactor/select-timeout-and-sleeps

Conversation

@ysbaddaden
Copy link
Collaborator

A couple refactors related to select timeout and the signature of Crystal::EventLoop::Event#add that only needs to handle a nilable for libevent (because it caches events); other loop don't need that.

Extracted from #14959

Sleep and registering a select action are always setting a Time::Span
and don't need to deal with a nilable.

The exception is IO::Evented that keeps a cache of libevent events and
must be able to remove the timeout in case @read_timeout would have been
set to nil before a second wait on read.
@ysbaddaden
Copy link
Collaborator Author

ysbaddaden commented Sep 6, 2024

Argh, it doesn't work on its own because IO::Evented isn't isolated to the libevent event loop (yet). Actually it's the WASI EventLoop that is built on top of IO::Evented 🤨

@straight-shoota straight-shoota added this to the 1.14.0 milestone Sep 7, 2024
@straight-shoota straight-shoota changed the title Refactor eventloop interface for sleeps & select timeouts Refactor EventLoop interface for sleeps & select timeouts Sep 9, 2024
@straight-shoota straight-shoota merged commit c8ecd93 into crystal-lang:master Sep 9, 2024
@ysbaddaden ysbaddaden deleted the refactor/select-timeout-and-sleeps branch September 13, 2024 13:26
@jkthorne
Copy link
Contributor

This PR actually changes the API. you have to pass 2 args to Fiber#timeout() and nil is no longer an option.

I am not saying this is the wrong choice but it did break a library. Should there be a PR to add a deprecation warning or something?

@ysbaddaden
Copy link
Collaborator Author

That method should actually be nodoc (like the related methods): there's no way to create a Channel::SelectAction that is a private type.

@straight-shoota
Copy link
Member

Yeah, this is technically a breaking change because the method was part of the public API. But I suppose it wont't be feasible to uphold this signature in a deprecated state because of the internal changes requiring a timeout_select_action for a timeout.

@jkthorne Could you tell a bit more about how/where this is used? Does it also cancel the timeout, and if so, how? Fiber.cancel_timeout is only available for the current fiber, so a suspended fiber needs to resume somehow in order to cancel a timeout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants