Skip to content

OpFinishHandle use Runtime::cancel()#84

Open
wokron wants to merge 5 commits intomasterfrom
finish-handle-use-external-cancel
Open

OpFinishHandle use Runtime::cancel()#84
wokron wants to merge 5 commits intomasterfrom
finish-handle-use-external-cancel

Conversation

@wokron
Copy link
Copy Markdown
Owner

@wokron wokron commented Apr 2, 2026

No description provided.

@wokron wokron force-pushed the finish-handle-use-external-cancel branch from 31b352d to 169821a Compare April 2, 2026 12:03
@wokron wokron requested a review from Copilot April 2, 2026 12:08
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors finish-handle cancellation to route io_uring cancel requests through Runtime::cancel() (instead of directly submitting a cancel SQE from the handle), introduces a shared OpFinishHandleBase header, and updates concepts/tests to match the new cancel(Runtime*) shape.

Changes:

  • Extract OpFinishHandleBase into include/condy/finish_handle_base.hpp and adjust runtime.hpp / finish_handles.hpp includes accordingly.
  • Update HandleLike and all affected finish handles to use cancel(Runtime*), with OpFinishHandle::cancel() delegating to Runtime::cancel().
  • Simplify many tests by replacing manual ring/event loops with condy::sync_wait.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
include/condy/runtime.hpp Stops including finish_handles.hpp; uses finish_handle_base.hpp while keeping cancel processing in the runtime event loop.
include/condy/finish_handles.hpp Moves cancellation to cancel(Runtime*), uses Runtime::cancel() for OpFinishHandle, and passes runtime through parallel finish handles.
include/condy/finish_handle_base.hpp New home for OpFinishHandleBase used by the runtime to dispatch CQEs.
include/condy/concepts.hpp Updates HandleLike to require cancel(Runtime*).
include/condy/channel.hpp Updates channel finish handles to accept Runtime* in cancel.
include/condy/channel_legacy.hpp Updates legacy channel finish handles to accept Runtime* in cancel.
tests/test_parallel_finish_handle.cpp Adapts test-only finish handle to cancel(Runtime*) and updates call sites.
tests/test_parallel_awaiter.cpp Adapts test-only finish handle to cancel(Runtime*) and adds runtime include.
tests/test_op_finish_handle.cpp Removes a cancellation test that depended on the previous “cancel via Context ring” behavior.
tests/test_op_awaiter.cpp Replaces manual event loop wiring with sync_wait and adjusts/selects new test coverage.
tests/test_awaiter_operations.cpp Replaces manual event loop wiring with sync_wait.
Comments suppressed due to low confidence (1)

include/condy/channel_legacy.hpp:428

  • cancel(Runtime* runtime) only uses runtime in an assert. In NDEBUG builds the parameter becomes unused, and the repo builds with -Wall -Wextra -Werror (CMakeLists.txt:59), so this can fail compilation due to -Wunused-parameter. Mark the parameter [[maybe_unused]] (like the PushFinishHandle version) or otherwise reference it outside the assert.
    void cancel(Runtime *runtime) noexcept {
        assert(runtime == runtime_);
        if (channel_->cancel_pop_(this)) {
            // Successfully canceled
            runtime_->resume_work();
            runtime_->schedule(this);
        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@wokron wokron force-pushed the finish-handle-use-external-cancel branch from 169821a to c2aa4ee Compare April 2, 2026 13:35
@wokron wokron requested a review from Copilot April 2, 2026 13:49
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@wokron wokron force-pushed the finish-handle-use-external-cancel branch from 8d64f73 to 3063349 Compare April 2, 2026 14:51
@wokron wokron marked this pull request as ready for review April 2, 2026 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants