Refactor cancellation of IOCP::OverlappedOperation
#14754
Conversation
| event_loop.dequeue(timeout_event) | ||
| unless @state.done? | ||
| if cancel! | ||
| Fiber.suspend |
There was a problem hiding this comment.
suggestion: maybe explain why we need to suspend the fiber?
There was a problem hiding this comment.
suggestion: we only need to cancel the operation when the fiber awoke from a timeout, so inside the if timeout branch above. The other branch will only ever resume the fiber when the operation was completed.
Or is that to protect against spurious wakeups?
There was a problem hiding this comment.
Yeah, the cancellation is only relevant when there is a timeout.
Dunno if it's worth keeping it for both branches as a sanity measure 🤔 I don't see any immediate reason to expect a spurious wakeup.
There was a problem hiding this comment.
I guess there could be use cases for external cancellation, though. We don't need to explicitly cancel pending operations upon closing a handle because IOCP does that automatically. But maybe there could be other situations where we want to cancel a pending operation from a different fiber? 🤔
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.
CANCELLEDstate. 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.sleepandsuspend@@cancelledlinked list.IOCP::OverlappedOperationinternalizehandleandwait_for_completion#14724 (comment)