fix: Prevent multiple effect invocations when reading the same signal multiple times#23978
fix: Prevent multiple effect invocations when reading the same signal multiple times#23978
Conversation
… multiple times When an effect reads the same signal multiple times (e.g. signal.get() called 3 times), each call creates a separate Usage instance and registers a separate listener. When the signal changes, all listeners fire, causing the effect to run multiple times instead of once. Added an AtomicBoolean flag in Effect.revalidate() that ensures onDependencyChange is called only once per change event, even if multiple usages refer to the same signal. The flag is reset on each revalidate() call so subsequent signal changes can trigger the effect again. Fixes #23974 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…gnal multiple times When an effect reads the same signal multiple times (e.g. signal.get() called 3 times), each call creates a separate Usage instance and would register a separate listener. This causes the effect to be invoked multiple times for a single signal change. Added getIdentity() method to Usage interface that returns an identity object for deduplication purposes. Effect now uses a Set of UsageWrapper objects (which implement equals/hashCode based on usage identity) to automatically deduplicate usages. Only the first occurrence of each unique signal identity gets a listener registered. CombinedUsage calculates its identity as a Set of all combined usage identities, providing proper identity semantics for combined usages. This approach prevents duplicate listeners from being registered at the data structure level, eliminating the need for separate tracking. Fixes #23974 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
mshabarov
changed the title
… same signal multiple times" This reverts commit 0b1b5c6.
mshabarov
changed the title
…o multiple-effect-invocations
|
tltv
left a comment
tltv
left a comment
There was a problem hiding this comment.
Bug: activate() has the same duplicate-invocation problem (not fixed)
Effect.java:304-305 — When re-activating without changes, listeners are re-registered directly:
registrations.add(usage.onNextChange(this::onDependencyChange));
This bypasses the changeHandled guard. If usages contains multiple entries for the same signal (from multiple reads before passivation), the same cascading problem occurs: the signal copies its listener list before iterating,
so clearRegistrations() during a synchronous invalidation doesn't prevent remaining copied listeners from firing.
Recommendation: Extract the changeHandled pattern into a shared method, or apply the same guard in activate().
| // between the hasChanges check and the onNextChange call, | ||
| // in which case firstRun is already set to true above. | ||
| for (UsageTracker.Usage usage : usages) { | ||
| registrations.add(usage.onNextChange(this::onDependencyChange)); |
There was a problem hiding this comment.
Bug: activate() has the same duplicate-invocation problem (not fixed)
When re-activating without changes, listeners are re-registered directly here.
This bypasses the changeHandled guard. If usages contains multiple entries for the same signal (from multiple reads before passivation), the same cascading problem occurs: the signal copies its listener list before iterating,
so clearRegistrations() during a synchronous invalidation doesn't prevent remaining copied listeners from firing.
Recommendation: Extract the changeHandled pattern into a shared method, or apply the same guard in activate().
| boolean[] hasSignalUsage = { false }; | ||
| // Ensure effect runs only once per change event, even if the same | ||
| // signal is read multiple times (each read registers a listener) | ||
| AtomicBoolean changeHandled = new AtomicBoolean(false); |
There was a problem hiding this comment.
nit: Unnecessary AtomicBoolean for non-concurrent use
2 participants
When an effect reads the same signal multiple times (e.g. signal.get()
called 3 times), each call creates a separate Usage instance and registers
a separate listener. When the signal changes, all listeners fire, causing
the effect to run multiple times instead of once.
Added an AtomicBoolean flag in Effect.revalidate() that ensures
onDependencyChange is called only once per change event, even if multiple
usages refer to the same signal. The flag is reset on each revalidate()
call so subsequent signal changes can trigger the effect again.
Fixes #23974
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com