-
Notifications
You must be signed in to change notification settings - Fork 43
Adds per-operation cancellation support to async_run and cleans up cancellation #321
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
Adds per-operation cancellation support to async_run and cleans up cancellation #321
Conversation
This is a big rework on how cancellation is handled in the connection. It has the side effect of implementing per-operation cancellation for async_run, but the main point is fixing race conditions and making the reader/writer not concerned about cancelling the connection. I prefer merging this before #320, which fixes the last race conditions regarding cancellation. Once this is done, I'll submit a PR implementing support for cancel_after in connection. |
self.complete(system::error_code{}); | ||
|
||
// Wait until we're cancelled. This simplifies parallel group handling a lot | ||
conn_->ping_timer_.expires_at((std::chrono::steady_clock::time_point::max)()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea. But I guess it would have been even better to disable health-checks by letting users set max()
instead of zero()
, then we would not need this branch at all. We would only have to reformulate the loop below to first wait and then to ping and not the other way around. That would be a breaking change hard to get around. We would have to rename health_check_interval
to something like health_check_interval2
to intentionally break user code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, that wouldn't work because the interval is a duration, not a time_point. I actually had a bug at some point where I used duration::max
rather than time_point::max
and it caused an overflow error, with the operation finishing immediately.
timer_type reconnect_timer_; // to wait the reconnection period | ||
timer_type ping_timer_; // to wait between pings | ||
receive_channel_type receive_channel_; | ||
asio::cancellation_signal run_signal_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this still be needed if we remove the cancel function a let would work entirely with user provided cancellation_signals
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it wouldn't. It only exists to keep compatibility with cancel()
. But I feel removing it is breaking too much (essentially, everyone).
Some many nice things. The simplifications on the reader_fsm are relieving. Good job, thanks. |
close #318
close #319