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 IOCP::OverlappedOperation internalize handle and wait_for_completion #14724

Conversation

straight-shoota
Copy link
Member

Moves the schedule_overlapped overlapped method into OverlappedOperation and call it directly from the result methods, reducing the external API.
@handle becomes a property of OverlappedOperation which will be necessary for a follow-up refactor anyway.

@straight-shoota straight-shoota force-pushed the refactor/iocp-operation-wait_for_completion branch from 2aef158 to a1fc21d Compare June 17, 2024 22:21
@straight-shoota straight-shoota added this to the 1.13.0 milestone Jun 18, 2024
@beta-ziliani beta-ziliani removed this from the 1.13.0 milestone Jun 24, 2024
@straight-shoota straight-shoota marked this pull request as draft June 24, 2024 12:33
@straight-shoota straight-shoota marked this pull request as ready for review June 24, 2024 21:03
@straight-shoota
Copy link
Member Author

I found a workaround for the special case of an accept timeout: b919f3b

This is quite simple and only temporary. The handling of timeouts will change significantly in a follow-up.

@straight-shoota straight-shoota added this to the 1.13.0 milestone Jun 25, 2024
@straight-shoota straight-shoota merged commit 7e33ecc into crystal-lang:master Jun 25, 2024
61 checks passed
@straight-shoota straight-shoota deleted the refactor/iocp-operation-wait_for_completion branch June 25, 2024 14:16
straight-shoota added a commit that referenced this pull request Aug 7, 2024
When an overlapped operation gets cancelled, we still need to wait for completion of the operation (with status `ERROR_OPERATION_ABORTED`) before it can be freed.
Previously we stored a reference to cancelled operations in a linked list and removed them when complete. This allows continuing executing the fiber directly after the cancellation is triggered, but it's also quite a bit of overhead.
Also it makes it impossible to allocate operations on the stack.

Cancellation is triggered when an operation times out.

The change in this patch is that after a timeout the fiber is suspended again, expecting completion via the event loop. Then the operation can be freed.

* Removes the `CANCELLED` state. It's no longer necessary, we only need to distinguish whether a fiber was woken up due to timeout or completion. A follow-up will further simplify the state handling.
* Replace special timeout event and fiber scheduling logic with generic `sleep` and `suspend`
* Drops `@@cancelled` linked list.
* Drops the workaround from #14724 (comment)
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