Skip to content

Fix: Correctly transfer FD ownership in polling event loop #15650

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

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Apr 10, 2025

An event loop only takes ownership the first time a fd is waited upon, and then never checks whether the current event loop owns the fd.

It causes issues where connecting a socket in one fiber then trying to read with a timeout in other fiber will never resume on IO ready, because we must be able to cancel the timer to be able to resume the fiber (oops).

It also causes cross event loop instances enqueues that are fine with preview MT (inefficient, but it would push the fiber to its thread), but invalid with execution context MT as it would transfer a fiber to another context 😨

closes #15647

It only takes ownership the first time a fd is waited upon, and then
never checked anymore.

It causes issues where connecting a socket in one fiber then trying to
read with a timeout in other fiber will never resume on IO ready,
because we must be able to cancel the timer to be able to resume the
fiber (oops).

It also causes cross event loop instances enqueues that are fine with
preview MT, but invalid with execution context MT.
@ysbaddaden ysbaddaden added the kind:bug A bug in the code. Does not apply to documentation, specs, etc. label Apr 10, 2025
@ysbaddaden ysbaddaden self-assigned this Apr 10, 2025
@straight-shoota straight-shoota added this to the 1.16.1 milestone Apr 10, 2025
@ysbaddaden ysbaddaden moved this from Review to Approved in Multi-threading Apr 10, 2025
@straight-shoota straight-shoota merged commit a6a223f into crystal-lang:master Apr 13, 2025
39 of 41 checks passed
@github-project-automation github-project-automation bot moved this from Approved to Done in Multi-threading Apr 13, 2025
@straight-shoota straight-shoota changed the title Fix: polling event loop doesn't transfer ownership Fix: Correctly transfer FD ownership in polling event loop Apr 13, 2025
@zw963
Copy link
Contributor

zw963 commented Apr 13, 2025

Does this fix this ?

AFAIK, above issue still happen on 1.16.0

@zw963
Copy link
Contributor

zw963 commented Apr 14, 2025

New regression issue #15658 since this PR was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:multithreading topic:stdlib:runtime
Projects
Status: Done
5 participants