Fix EventBus instances being unexpectedly started up after shutdown in certain conditions, fixes #102 (#103)

Author: PaintNinja

eventbus-test/src/test/java/net/minecraftforge/eventbus/test/IndividualEventListenerTests.java

@@ -4,6 +4,7 @@
  */
 package net.minecraftforge.eventbus.test;
 
+import net.minecraftforge.eventbus.api.bus.BusGroup;
 import net.minecraftforge.eventbus.api.bus.CancellableEventBus;
 import net.minecraftforge.eventbus.api.bus.EventBus;
 import net.minecraftforge.eventbus.api.event.RecordEvent;
@@ -316,4 +317,43 @@ record CancellableTestEvent() implements Cancellable, RecordEvent {
         CancellableTestEvent.BUS.removeListener(listener);
         Assertions.assertFalse(CancellableTestEvent.BUS.hasListeners(), "CancellableTestEvent.BUS should no longer have listeners");
     }
+
+    /**
+     * Tests that {@link BusGroup#shutdown()} and {@link BusGroup#startup()} work as expected.
+     */
+    @Test
+    public void testShutdownAndStartup() {
+        record TestEvent() implements RecordEvent {
+            static final BusGroup TEST_GROUP = BusGroup.create(TestEvent.class.getName() + "testShutdown");
+            static final EventBus<TestEvent> BUS = EventBus.create(TEST_GROUP, TestEvent.class);
+        }
+
+        var called = new AtomicBoolean();
+        Assertions.assertFalse(called.get(), "Listeners should not have been called yet");
+        var listener1 = TestEvent.BUS.addListener(event -> called.set(true));
+        var listener2 = TestEvent.BUS.addListener(event -> called.set(true));
+        Assertions.assertFalse(called.get(), "Listeners should not have been called yet");
+        TestEvent.BUS.post(new TestEvent());
+        Assertions.assertTrue(called.get(), "Listeners should have been called");
+
+        called.set(false);
+        TestEvent.TEST_GROUP.shutdown();
+        TestEvent.BUS.post(new TestEvent());
+        Assertions.assertFalse(called.get(), "Listeners should not have been called after shutdown");
+
+        TestEvent.BUS.removeListener(listener1);
+        TestEvent.BUS.post(new TestEvent());
+        Assertions.assertFalse(called.get(), "Listener2 should not have been called after removal of Listener1");
+
+        TestEvent.BUS.removeListener(listener2);
+        TestEvent.BUS.addListener(listener1);
+        TestEvent.BUS.post(new TestEvent());
+        Assertions.assertFalse(called.get(), "Listener1 should not have been called after adding Listener2 back when the bus is shutdown");
+
+        TestEvent.TEST_GROUP.startup();
+        TestEvent.BUS.post(new TestEvent());
+        Assertions.assertTrue(called.get(), "Listener1 should have been called after startup");
+
+        TestEvent.TEST_GROUP.dispose();
+    }
 }

src/main/java/net/minecraftforge/eventbus/internal/AbstractEventBusImpl.java

@@ -139,7 +139,9 @@ default void startup() {
 
         synchronized (backingList()) {
             // Force invalidate the invoker to remove the no-op invoker that might've been set by shutdown()
-            alreadyInvalidated().set(false);
+            // Note: Opaque suffices here as this write immediately precedes the read made in invalidateInvoker() inside
+            //       the same synchronised block
+            alreadyInvalidated().setOpaque(false);
             invalidateInvoker();
 
             children().forEach(AbstractEventBusImpl::startup);

src/main/java/net/minecraftforge/eventbus/internal/CancellableEventBusImpl.java

@@ -122,6 +122,7 @@ public boolean hasListeners() {
 
     @Override // overrides from AbstractEventBusImpl
     public void invalidateInvoker() {
+        if (alreadyInvalidated.getAcquire()) return;
         invokerCallSite.setTarget(backingList.isEmpty() ? MH_NO_OP_PREDICATE : MH_NULL_PREDICATE);
     }
 

src/main/java/net/minecraftforge/eventbus/internal/EventBusImpl.java

@@ -87,6 +87,7 @@ public boolean hasListeners() {
 
     @Override // overrides from AbstractEventBusImpl
     public void invalidateInvoker() {
+        if (alreadyInvalidated.getAcquire()) return;
         invokerCallSite.setTarget(backingList.isEmpty() ? MH_NO_OP_CONSUMER : MH_NULL_CONSUMER);
     }