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

Already on GitHub? Sign in to your account

Refactor EventLoop interface for sleeps & select timeouts #14980

Conversation

ysbaddaden
Copy link
Contributor

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
Contributor 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
63 of 65 checks passed
@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
Contributor 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