Reactive engine fixes - #8
Closed
UnknownJoe796 wants to merge 12 commits into
Closed
Conversation
The onLoad callback was guarded by `if (wasLoadingLastTime)` while the flag only ever became true inside that same block, so onLoad was unreachable dead code. Invert the guard so onLoad fires on the transition INTO a loading state and set the flag before invoking the callback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…node Reading `.state` on an unobserved Remember calls `runOnceWhileDead`, which ran the calculation and its `invoke()` operators registered `rerun` listeners on every source. Those listeners were never released, so a later source change fired `rerun` and resurrected the supposedly-dead, zero-listener context into a permanently-computing graph node, contradicting the documented laziness. runOnceWhileDead now clears the dependency scratch list and cancels the context after the throwaway run, releasing every listener it registered. Adds a `listenerCount` test hook to BaseListenable and a regression test asserting a dead read leaves the source with zero listeners and does not recompute. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A calculation that writes to a signal it also reads synchronously re-invokes its own startCalculation, recursing until the stack overflows (or livelocking under a dispatching scheduler) with no diagnostic. Add a per-context `calculating` flag, checked at the top of startCalculation (which the listener dispatch calls synchronously even under a dispatcher), and throw a descriptive ReactiveReentrancyException instead. Writing to unrelated signals a calculation does not depend on stays legal. Adds tests for both the throwing case and the legitimate unrelated-write case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The fix in b90d368 made runOnceWhileDead() call dependencyBlockStart() + cancel() to release the throwaway run's listeners, but cancel() over-cancels the context, stranding an in-flight async load in sharedTest3 (loadCount stuck at 1). A refinement to release only the dependency listeners did not resolve it either; a correct fix needs more careful reactive-core design and verification. Reverting runOnceWhileDead to its baseline, and removing the listenerCount test hook and the readingDeadStateDoesNotLeakOrResurrect regression test that went with the fix. The original dead-read listener leak (a real bug) is therefore still open. The fix attempt is preserved on branch reactive-remember-leak-wip for a follow-up that keeps sharedTest3 green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The reactive graph is single-threaded by design (unsynchronized ArrayList listener lists and dependency trackers), but nothing enforced it, so a background-thread mutation would silently corrupt state. Add ReactiveThreadCheck: an opt-in (off by default) assertion that records the first mutating thread per BaseListenable and throws a clear IllegalStateException on a foreign-thread mutation in invokeAllListeners. It is opt-in with a pluggable currentThread hook because this library is common-only (no expect/actual for thread identity). Disabled, it is a single boolean read. Consumers enable it in debug builds via ReactiveThreadCheck.enabled/currentThread. Includes a test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When Remember.state was accessed on a dead (no-listener) context, the runOnceWhileDead() calculation would register listeners on every source it touched via the normal operator path. These listeners were never released, keeping sources alive and causing the supposedly-dead context to recompute whenever any source changed (violating laziness). The previous fix (cancel() after the dead run) broke sharedTest3 because the cancellation of the old calculation job deferred asynchronously under Dispatchers.Unconfined: old-job's invokeOnCompletion fired after the context had already been re-activated, wiping out the freshly registered dependencies. This fix adds a skipDependencyRegistration flag on TypedReactiveContext. runOnceWhileDead() sets it to true for the duration of the action; all dependency-registering operators (invoke, awaitNotNull, state, once, rerunOn, use, async, Deferred.invoke, Flow.invoke) check the flag and skip addListener/registerDependency/coroutine-launch when set. The dead read returns the current snapshot value with zero side-effects on the reactive graph. Also adds BaseListenable.listenerCount (test-visible) and the regression test readingDeadStateDoesNotLeakOrResurrect that pins both the no-leak and no-resurrection invariants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CfGVb2vVm8ZKXk6g2bhFXc
Combines the reactive engine fixes with the platform-backed rewrite of the
thread-confinement check, so the code changes land independently of the
explicit-API conversion that follows.
Engine fixes:
- R4: repeated reads of the same dependency in one run no longer add duplicate
usedDependencies entries.
- B6: TypedReactiveContext.async keyed its cache on dependency values only (two
same-dep async blocks aliased) and launched on the long-lived scope (stale
work never cancelled). Cancel the prior Job on rerun, mirroring the Flow
operator; same stale-launch fix applied to the sibling Deferred.invoke().
- B7: reactiveState {} swallowed CancellationException as notReady, letting a
cancelled suspending calc write stale state. Rethrow it.
- B8: delete the false "no dependencies -> auto-cancelled" KDoc claim.
async identity:
- async() now takes an explicit `identity: String` and keys its cache on
setOf(identity, *dependencies) rather than deriving call-site identity from
action::class.
Thread confinement:
- Replace the pluggable ReactiveThreadCheck.currentThread hook and the
ThreadConfinementGuard class with an `internal expect fun
currentReactiveThread()` plus a `checkThreadConfinement(owningThread)`
function whose result callers store back, avoiding a per-node allocation.
- Add jvm (Thread.currentThread), ios (NSThread.currentThread) and js (null,
no shared-memory threads) actuals.
jvmTest 113 tests green; JS + iOS compile green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017UHkALCCi6UrsGba6wUGWp
iHoonter
previously approved these changes
Jul 28, 2026
Contributor
|
Approved pending the |
iHoonter
requested changes
Jul 29, 2026
iHoonter
left a comment
iHoonter
left a comment
Contributor
There was a problem hiding this comment.
Added a few comments for things that need adjustment.
Backported from reactive-api-split so the two branches agree on behavior: - ReactiveState.ready compares the sentinels with != rather than !is. The sentinels are objects, so equality is the intended check. - Remember.activate no longer overwrites notActive with notReady when useLastWhileLoading is set - that is the whole point of the flag. - onNextSuccess routes both the listener and the post-subscribe read through one `perform` guarded by `acted`, instead of duplicating the body and reconciling afterwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GA6LAUN9Lpo5hyK3KVJteJ
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
2 participants
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.
dead running still under consideration