Repository navigation
fix: timeout callbacks hold their session, not this - #44
Merged
Merged
Conversation
Timer promised that a callback that has been replaced or cancelled is never called. On ASIO, that does not hold for a wait whose delay has expired already: its handler is queued with success, and arm(), cancel() or the destructor cannot take it back -- the contract of every asynchronous operation. Timer now says so, and leaves lifetime to the callback. Three callbacks referred to what they cancel, [this] and [&stream], and could run after the coroutine that armed them had moved on, against a session that may be gone. The h2 and h2v2 shutdowns now capture weak_from_this() and cancel through it; io::drain() takes the owner of its stream and does nothing once the owner is gone. The timer test of test_corosio.cpp moves to test_runtime.cpp, to run on both runtimes. Closes #43 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #43, but not the way the issue proposed:
Timerkeeps Asio's contract, and the callbacks that relied on more are fixed instead.Cancelling an asynchronous operation means it completes soon, cancelled or successfully. A wait whose delay has expired already completes successfully, so its callback still runs once after
arm(),cancel()or the destructor. A stop token per wait would hide that, at the cost of an allocation on everyarm(), which runs after every packet on QUIC. The Corosio runtime gives no more than Asio either: the extra check is only in anyhttp's CorosioTimerwrapper, see the issue.Changes
Timer(asio/runtime.hpp): the code is unchanged. The doc comment no longer promises that a cancelled or replaced callback is "never called". It says the callback may run once late, so it must hold what it uses and do no harm.shutdown(): the timeout no longer capturesthis, but[weak = weak_from_this()], and cancels throughselfonceweak.lock()succeeds.io::drain(): takes the owner of its stream as astd::weak_ptr<const void>and does nothing once it is gone. The callers in h1, h2 and h2v2 passweak_from_this().Timerre-arm/cancel test moves fromtest_corosio.cpptotest_runtime.cpp, so it runs on both runtimes.Testing
Full suites pass in
build/(ASIO),build-corosio/andbuild-h2v2/. The timeouts still fire through the weak pointers:TlsConnectionEnd.WHEN_client_never_answers_the_close_notify_THEN_server_ends_anyway: about 2.0 s.ClientTlsEnd.WHEN_write_is_stuck_THEN_shutdown_interrupts_it_and_says_so/HTTP2: about 2.7 s.The GCC and ASAN trees have not been run on this version.
🤖 Generated with Claude Code