fix: resolve event dispatch divergence in ObserverManager and add tests - #15
Open
jeanouii wants to merge 1 commit into
Open
fix: resolve event dispatch divergence in ObserverManager and add tests#15jeanouii wants to merge 1 commit into
jeanouii wants to merge 1 commit into
Conversation
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.
Event dispatch: make @event Consumer consistent with fireEvent
Summary
Publishing the same event object reaches different observers depending on how it is published.
Both paths ultimately walk up the type hierarchy ("this type + its ancestors"). The only difference is the starting type: the runtime class for fireEvent, versus the statically captured T for a Consumer. Starting the walk at a supertype can never reach an observer registered under a subtype.
Why this is a problem
An injected @event Consumer (or any Consumer) silently skips observers registered on a subtype. For example, an observer void observe(@observes PatchCreated e) is never invoked when the event is published through a Consumer, even though the object being published is a PatchCreated. The exact same event published via system.fireEvent(patchCreated) reaches it.
The two publish styles look interchangeable but are not, and the failure is silent — no error, the observer just never runs. Injecting a broad Consumer "so everyone receives it" does the opposite of what the type suggests.
Why the per-consumer AtomicReference cache can be dropped
ConsumerReference today caches its resolved Invocation in a per-instance AtomicReference, and this cache is the direct cause of the divergence: it is computed once against the static T and reused, so dispatch is pinned to T instead of the event's runtime class.
The cache is also redundant. getInvocation(Class) already caches per class in the shared methods map, so the AtomicReference is a cache on top of a cache — it saves at most one map.get per publish. To keep it coherent when observers change, addObserver/removeObserver must invalidate both caches (methods.clear() and iterate every ConsumerReference to clear() it), which is why the references list and the per-consumer clear() exist at all. Because accept bypasses fireEvent, it also has to re-duplicate fireEvent's finally { seen.remove() } re-entrancy cleanup.
Removing the per-consumer cache removes all of that machinery (the AtomicReference, resolve, clear, the references list, and its two invalidation calls) for negligible cost, while fixing the correctness bug.
Proposed resolution
Make Consumer.accept(e) equivalent to system.fireEvent(e) — dispatch on the runtime type, keeping T only as a compile-time bound plus a runtime assertion:
This does not resolve observers for T and its subtypes — that would over-deliver (handing an AccountCreated to an @observes AccountUpdated sibling handler). Dispatch stays "runtime type, walking up," exactly what fireEvent already does.
Tests
EventDispatchDivergenceTest pins the contract across both publish paths (fireEvent and Consumer) with an exact / sibling-subtype / supertype matrix, plus a separate Object observer to confirm cross-observer delivery is unaffected. Full suite: 332 tests green.