docs: correct the post-#151 QtExecutor teardown claims, and pin what dropping costs - #256
Merged
Merged
Conversation
…dropping costs Six comments still described QtExecutor's pre-#151 behaviour: that it is stateless, and that callbacks it has already posted "stay safe" after their owner is gone. Since #151 it carries an `_alive` token and a queued task is dropped, not run. docs/spec/core/executor.md was self-contradictory on its own page -- §Teardown says tasks are dropped, and forty-one lines later the QtExecutor section said it has "no lifetime or shutdown concerns of its own". Fixed: executor.md's stateless claim; the "stays safe" sentence in bookmarks' and pastebin's app.hpp; pastebin's "they detonate whenever some later processEvents() gets to them"; and main.cpp's "a callback delivered after ~App is a use-after-free" -- after ~App there is no delivery at all. One site more than #194 lists: examples/common/testkit/backend_rig.hpp carried the same "QtExecutor is stateless ... callbacks stay safe after the rig is gone" sentence. It is stale for the same reason and is corrected here. Two superficially identical sites are deliberately left alone, because they are still true: backend_rig.hpp's and TESTING.md's "the stale callback ... detonates inside whatever later test pumps it" describe a pool thread calling post() on a *freed IExecutor\**, a member call on destroyed memory. `_alive` guards delivery of an already-queued event; it cannot guard that, and #151 did not claim to. The declaration-order rationale those comments justify stays load-bearing, so only the trailing "stays safe" sentence was removed from each. The consequence of dropping was documented nowhere and is observable: the decrement lives inside the .then/.onError body, and the `_alive` guard returns before invoking it, so a dropped completion releases its shared_ptr copy of the counter without decrementing. bookmarks' `_fetchInFlight` and pastebin's `_sweepInFlight` now state that as an invariant, along with why a dropped completion should *not* decrement: dropping is deliberate, a decrementing hook would run app teardown code from inside framework teardown, and no consumer can observe the difference -- the only reader of the counter is a drain that finishes strictly before ~App. Rather than leave that invariant as unguarded prose, tests/qt/ test_qt_executor_teardown.cpp now pins it, reproducing the apps' exact pattern. Verified independently rather than taken from the issue: dropped .then and dropped .onError both leave the counter at 1, a delivered completion takes it to 0. Mutation-checked -- pumping before the executor dies, so the callback is delivered instead of dropped, fails the assertion (0 == 1), so it measures the drop and not merely a counter nobody touches. No behaviour change. morph_qt_tests 503 assertions / 74 cases; pastebin+bookmarks ladder 173/173; Doxygen builds clean. Closes #194
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Closes #194.
The stale claims
Six comments still described
QtExecutor's pre-#151 behaviour — that it isstateless, and that callbacks it has already posted "stay safe" after their owner
is gone. Since #151 it carries an
_alivetoken, and the guard returns beforeinvoking the callback body, so a queued task is dropped, not run.
docs/spec/core/executor.mdwas contradicting itself on its own page: §Teardownsays tasks are dropped, and forty-one lines later the
QtExecutorsection said ithas "no lifetime or shutdown concerns of its own".
Corrected:
docs/spec/core/executor.mdexamples/bookmarks/…/app.hppAppis gone"examples/pastebin/…/app.hppexamples/pastebin/…/app.hppprocessEvents()gets to them"examples/pastebin/src/server/main.cpp~Appis a use-after-free"examples/common/testkit/backend_rig.hppTwo sites deliberately left alone
backend_rig.hppandexamples/TESTING.mdboth say a stale callback "detonatesinside whatever later test pumps it". That reads like the pastebin sentence I
did remove, but it is still true: it describes a pool thread calling
post()on a freedIExecutor*— a member call on destroyed memory._aliveguards delivery of an already-queued event; it cannot guard that, and#151 never claimed to.
The declaration-order rationale those comments justify therefore stays fully
load-bearing, which is why only the trailing "stays safe" sentence came out of
each — not the paragraph around it.
What dropping actually costs
Documented nowhere, and observable: the decrement lives inside the
.then/.onErrorbody, so a dropped completion releases itsshared_ptrcopy ofthe counter without ever decrementing.
_fetchInFlight/_sweepInFlightnowcarry that as a stated invariant, together with why a dropped completion should
not decrement — dropping is deliberate, a decrementing hook would run
application teardown code from inside framework teardown, and no consumer can
observe the difference, since the only reader is a drain that finishes strictly
before
~App.Pinned, not just asserted
An invariant stated only in prose is what drifted here in the first place, so
tests/qt/test_qt_executor_teardown.cppnow reproduces the apps' exact pattern.I measured this myself rather than carrying over the issue's numbers:
.then→ counter stays1.onError→ counter stays10Mutation-checked: pumping before the executor dies, so the callback is
delivered rather than dropped, fails the assertion (
0 == 1). The test measuresthe drop, not merely a counter nobody touches.
Verification
No behaviour change — comments, spec prose, and new tests only.
morph_qt_testsWARN_AS_ERROR)