From 3ea6de3686886ee7f8171b5811da9d82e40332eb Mon Sep 17 00:00:00 2001 From: Jean-Louis Monteiro Date: Thu, 30 Jul 2026 15:43:12 +0200 Subject: [PATCH] fix: resolve event dispatch divergence in ObserverManager and add tests --- .../pixie/observer/ObserverManager.java | 36 +--- .../pixie/EventDispatchDivergenceTest.java | 156 ++++++++++++++++++ 2 files changed, 165 insertions(+), 27 deletions(-) create mode 100644 src/test/java/org/tomitribe/pixie/EventDispatchDivergenceTest.java diff --git a/src/main/java/org/tomitribe/pixie/observer/ObserverManager.java b/src/main/java/org/tomitribe/pixie/observer/ObserverManager.java index 43e5c8c..fdce5f9 100644 --- a/src/main/java/org/tomitribe/pixie/observer/ObserverManager.java +++ b/src/main/java/org/tomitribe/pixie/observer/ObserverManager.java @@ -40,7 +40,6 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import java.util.logging.Level; @@ -59,7 +58,6 @@ protected Set initialValue() { private static final AtomicReference LOGGER = new AtomicReference<>(); private final Set observers = new LinkedHashSet<>(); private final Map methods = new ConcurrentHashMap<>(); - private final List references = new CopyOnWriteArrayList<>(); public boolean addObserver(final Object observer) { @@ -71,7 +69,6 @@ public boolean addObserver(final Object observer) { final Observer wrapper = new Observer(observer); if (wrapper.hasObserverMethods() && observers.add(wrapper)) { methods.clear(); - references.stream().forEach(ConsumerReference::clear); fireEvent(new ObserverAdded(observer)); return true; } else { @@ -89,7 +86,6 @@ public boolean removeObserver(final Object observer) { try { if (observers.remove(new Observer(observer))) { methods.clear(); - references.stream().forEach(ConsumerReference::clear); fireEvent(new ObserverRemoved(observer)); return true; } else { @@ -114,15 +110,12 @@ public E fireEvent(final E event) { public Consumer consumersOf(final Class eventClass) { if (eventClass == null) throw new IllegalArgumentException("eventClass cannot be null"); - final ConsumerReference e = new ConsumerReference(eventClass); - references.add(e); - return e; + return new ConsumerReference(eventClass); } private class ConsumerReference implements Consumer { private final Class type; - private final AtomicReference invocation = new AtomicReference<>(); private ConsumerReference(final Class type) { if (type == null) throw new IllegalArgumentException("type cannot be null"); @@ -131,32 +124,21 @@ private ConsumerReference(final Class type) { @Override public void accept(final E e) { - try { - getInvocation().invoke(e); - - } finally { - seen.remove(); + if (e != null && !type.isInstance(e)) { + throw new IllegalArgumentException( + "event " + e.getClass().getName() + " is not a " + type.getName()); } - } - - private Invocation getInvocation() { - return this.invocation.updateAndGet(this::resolve); - } - - private Invocation resolve(final Invocation invocation) { - if (invocation != null) return invocation; - return ObserverManager.this.getInvocation(type); - } - - private void clear() { - invocation.set(null); + // Dispatch on the runtime type, exactly like fireEvent, so observers registered + // on a subtype of the static type T are reached. The static type is kept only as + // a compile-time bound and the runtime assertion above. + fireEvent(e); } @Override public String toString() { return "ConsumerReference{" + "type=" + type.getName() + - "} " + getInvocation(); + "} " + ObserverManager.this.getInvocation(type); } } diff --git a/src/test/java/org/tomitribe/pixie/EventDispatchDivergenceTest.java b/src/test/java/org/tomitribe/pixie/EventDispatchDivergenceTest.java new file mode 100644 index 0000000..fd0f07c --- /dev/null +++ b/src/test/java/org/tomitribe/pixie/EventDispatchDivergenceTest.java @@ -0,0 +1,156 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.tomitribe.pixie; + +import org.junit.Assert; +import org.junit.Test; + +import java.util.function.Consumer; + +/** + * Reproduces the divergence described in HANDOFF-event-dispatch-divergence.md. + * + * Publishing the SAME event object reaches DIFFERENT observers depending on how + * it is published: + * + * - system.fireEvent(event) -> dispatches on event.getClass() (runtime type) + * - @Event Consumer bus; bus.accept(event) -> dispatches on the static generic T + * + * A Consumer therefore starts its up-walk at Object and never reaches an + * observer registered under a subtype such as PatchCreated. + * + * These tests assert the EXPECTED (consistent) behaviour: both publish paths should + * dispatch on the event's runtime type. They FAIL today, pinning the bug. + * + * The event hierarchy also has a SIBLING subtype PatchUpdated (both extend PatchChanged). + * Publishing a PatchCreated must never reach an observer registered only on PatchUpdated: + * dispatch walks UP the hierarchy (to supertypes), never sideways to siblings nor down to + * subtypes. This guards against the "over-deliver" fix the handoff warns against. + */ +public class EventDispatchDivergenceTest extends Assert { + + /** Control: fireEvent dispatches on the runtime type, so the concrete observer fires. */ + @Test + public void fireEventReachesConcreteObserver() { + final System system = System.builder() + .definition(Probe.class) + .definition(Logger.class) + .definition(Publisher.class) + .build(); + final Probe probe = system.get(Probe.class); + final Logger logger = system.get(Logger.class); + final Publisher publisher = system.get(Publisher.class); + final int anyBefore = probe.any; + final int loggerBefore = logger.any; + + publisher.fireEvent(new PatchCreated()); + + assertEquals("concrete observer should fire via fireEvent", 1, probe.created); + assertEquals("sibling observer must not fire for a PatchCreated", 0, probe.updated); + assertEquals("Object observer in the SAME object is shadowed by the more-specific onCreated", + anyBefore, probe.any); + assertEquals("Object observer in a SEPARATE component should still fire", + loggerBefore + 1, logger.any); + } + + /** + * Bug: publishing a PatchCreated through an injected Consumer dispatches on + * the static type Object, so the @Observes PatchCreated observer is never invoked. + * + * Expected behaviour (asserted here) mirrors fireEvent: the concrete observer fires. + * FAILS today (probe.created == 0). + */ + @Test + public void consumerOfObjectReachesConcreteObserver() { + final System system = System.builder() + .definition(Probe.class) + .definition(Logger.class) + .definition(Publisher.class) + .build(); + final Probe probe = system.get(Probe.class); + final Logger logger = system.get(Logger.class); + final Publisher publisher = system.get(Publisher.class); + final int anyBefore = probe.any; + final int loggerBefore = logger.any; + + publisher.accept(new PatchCreated()); + + assertEquals("concrete observer should fire via Consumer", 1, probe.created); + assertEquals("sibling observer must not fire for a PatchCreated", 0, probe.updated); + assertEquals("Object observer in the SAME object is shadowed by the more-specific onCreated", + anyBefore, probe.any); + assertEquals("Object observer in a SEPARATE component should still fire", + loggerBefore + 1, logger.any); + } + + // --- event hierarchy: PatchCreated extends PatchChanged (mirrors harminie) --- + + public static class PatchChanged { + } + + public static class PatchCreated extends PatchChanged { + } + + public static class PatchUpdated extends PatchChanged { + } + + public static class Probe { + private int created; + private int updated; + private int any; + + public void onCreated(@Observes final PatchCreated e) { + created++; + } + + public void onUpdated(@Observes final PatchUpdated e) { + updated++; + } + + public void onAny(@Observes final Object e) { + any++; + } + } + + /** + * A SEPARATE observer (distinct from Probe) that catches every event via @Observes Object. + * Because most-specific matching is per-observer, this one is NOT shadowed by Probe.onCreated: + * a distinct observer contributes its own best match, so it should fire for a PatchCreated too. + */ + public static class Logger { + private int any; + + public void onAny(@Observes final Object e) { + any++; + } + } + + public static class Publisher { + private final Consumer bus; + private final System system; + + public Publisher(@Event final Consumer bus, @Component @Param("system") final System system) { + this.bus = bus; + this.system = system; + } + + public void accept(final Object e) { + bus.accept(e); + } + + public void fireEvent(final Object e) { + system.fireEvent(e); + } + } +} \ No newline at end of file