Skip to content

fix: resolve event dispatch divergence in ObserverManager and add tests - #15

Open
jeanouii wants to merge 1 commit into
tomitribe:mainfrom
jeanouii:fix/event-dispatch-divergence-repro-test
Open

fix: resolve event dispatch divergence in ObserverManager and add tests#15
jeanouii wants to merge 1 commit into
tomitribe:mainfrom
jeanouii:fix/event-dispatch-divergence-repro-test

Conversation

@jeanouii

@jeanouii jeanouii commented Jul 30, 2026

Copy link
Copy Markdown
Member

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:

  @Override                                                                                                                                                                                                                                                         
  public void accept(final E e) {                                                                                                                                                                                                                                   
      if (e != null && !type.isInstance(e)) {                                                                                                                                                                                                                       
          throw new IllegalArgumentException(                                                                                                                                                                                                                       
                  "event " + e.getClass().getName() + " is not a " + type.getName());                                                                                                                                                                               
      }                                                                                                                                                                                                                                                             
      fireEvent(e); // dispatch on e.getClass() — same resolution + seen.remove() handling                                                                                                                                                                          
  }          

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant