Merge pull request boskworks#84 from prdoyle/tidy-hooks

Isolate `ThreadLocal`s in hooks

Author: prdoyle

bosk-core/src/main/java/works/bosk/Bosk.java

@@ -11,6 +11,9 @@
 import java.util.Queue;
 import java.util.concurrent.ConcurrentLinkedDeque;
 import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
 import java.util.concurrent.Semaphore;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.BiConsumer;
@@ -89,6 +92,7 @@ public class Bosk<R extends StateTreeNode> implements BoskInfo<R> {
 	private final RootRef rootRef;
 	private final ThreadLocal<R> rootSnapshot = new ThreadLocal<>();
 	private final Queue<HookRegistration<?>> hooks = new ConcurrentLinkedQueue<>();
+	private final ExecutorService hookExecutor = Executors.newVirtualThreadPerTaskExecutor();
 	private final PathCompiler pathCompiler;
 
 	// Mutable state
@@ -542,8 +546,23 @@ private void drainQueueIfAllowed() {
 				if (hookExecutionPermit.tryAcquire()) {
 					try {
 						for (Runnable ex = hookExecutionQueue.pollFirst(); ex != null; ex = hookExecutionQueue.pollFirst()) {
-							ex.run();
+							// Run the task in a separate virtual thread to prevent ThreadLocals from propagating.
+							// This is slightly tragic, because usually ThreadLocal propagation works just the
+							// way we'd want, but not always. Given the choices "always, sometimes, never", if
+							// we can't achieve "always", then the bosk philosophy prefers "never" over "sometimes".
+							hookExecutor.submit(ex).get();
 						}
+					} catch (ExecutionException e) {
+						if (e.getCause() instanceof RuntimeException r) {
+							throw r;
+						} else if (e.getCause() instanceof Error error) {
+							throw error;
+						} else {
+							throw new AssertionError("Hook runnable should catch and wrap checked exceptions", e);
+						}
+					} catch (InterruptedException e) {
+						LOGGER.warn("Interrupted while running hooks", e);
+						return;
 					} finally {
 						hookExecutionPermit.release();
 					}
@@ -622,7 +641,7 @@ public <T> void registerHook(String name, @NonNull Reference<T> scope, @NonNull
 
 	@Override
 	public void registerHooks(Object receiver) throws InvalidTypeException {
-		HookRegistrar.registerHooks(receiver, this);
+		HookScanner.registerHooks(receiver, this);
 	}
 
 	@Override

…/main/java/works/bosk/HookRegistrar.java …rc/main/java/works/bosk/HookScanner.javabosk-core/src/main/java/works/bosk/HookRegistrar.java renamed to bosk-core/src/main/java/works/bosk/HookScanner.java bosk-core/src/main/java/works/bosk/HookRegistrar.java renamed to bosk-core/src/main/java/works/bosk/HookScanner.java

@@ -7,7 +7,6 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.function.Function;
-import lombok.RequiredArgsConstructor;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import works.bosk.annotations.Hook;
@@ -17,8 +16,10 @@
 import static java.lang.reflect.Modifier.isStatic;
 import static works.bosk.util.ReflectionHelpers.getDeclaredMethodsInOrder;
 
-@RequiredArgsConstructor
-class HookRegistrar {
+/**
+ * Finds methods annotated with {@link Hook} in the given {@code object} and registers them in the given {@link Bosk}.
+ */
+class HookScanner {
 	static <T> void registerHooks(T receiverObject, Bosk<?> bosk) throws InvalidTypeException {
 		int hookCounter = 0;
 		for (
@@ -91,5 +92,5 @@ static <T> void registerHooks(T receiverObject, Bosk<?> bosk) throws InvalidType
 		}
 	}
 
-	private static final Logger LOGGER = LoggerFactory.getLogger(HookRegistrar.class);
+	private static final Logger LOGGER = LoggerFactory.getLogger(HookScanner.class);
 }

bosk-core/src/test/java/works/bosk/HooksTest.java

@@ -4,6 +4,8 @@
 import java.lang.reflect.InvocationTargetException;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.atomic.AtomicBoolean;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -78,6 +80,22 @@ void basic_hooksRunWhenRegistered() {
 			"Hook should fire when it's registered");
 	}
 
+	@Test
+	void basic_hooksDoNotPropagateThreadLocals() throws InterruptedException {
+		ThreadLocal<String> threadLocal = ThreadLocal.withInitial(() -> "initial");
+		threadLocal.set("updated");
+		BlockingQueue<String> queue = new LinkedBlockingQueue<>();
+		bosk.registerHook("foo", bosk.rootReference(), ref -> {
+			try {
+				queue.put(threadLocal.get());
+			} catch (InterruptedException e) {
+				throw new AssertionError("Huh?", e);
+			}
+		});
+		String observed = queue.take();
+		assertEquals("initial", observed, "Thread locals should not propagate into hooks");
+	}
+
 	@ParameterizedTest
 	@EnumSource(Variant.class)
 	void basic_noIrrelevantHooks(Variant variant) {