diff --git a/changelog.d/4448-injector-cfthread-resolving.fixed.md b/changelog.d/4448-injector-cfthread-resolving.fixed.md new file mode 100644 index 0000000000..c4f7d0c84b --- /dev/null +++ b/changelog.d/4448-injector-cfthread-resolving.fixed.md @@ -0,0 +1 @@ +- The dependency injector no longer throws a spurious `Wheels.DI.CircularDependency` when two of a request's `cfthread`s resolve the same transient (or request-scoped) service at the same time; the circular-dependency guard is now keyed per thread while keeping its per-request scope (#4448) diff --git a/vendor/wheels/Injector.cfc b/vendor/wheels/Injector.cfc index 510ab70e21..3557c15563 100644 --- a/vendor/wheels/Injector.cfc +++ b/vendor/wheels/Injector.cfc @@ -304,14 +304,12 @@ component implements="wheels.interfaces.di.InjectorInterface" { required string componentPath, required struct initArguments ) { - // Circular dependency guard. The resolving struct is request-scoped — - // it tracks "what this thread is currently resolving" — so concurrent - // requests don't trip each other's guard. Application-scoping it (the - // previous behavior) caused spurious self-loop errors when two - // requests near-simultaneously hit getInstance for the same controller - // on a cold framework, before Lucee had compiled the CFC. The chain - // "X -> X" in the error message was thread A's in-flight entry being - // observed by thread B before B even started. See issue #2331. + // Circular dependency guard. The resolving stack is per-thread (within a request-scoped outer + // struct) — so it tracks only what THIS thread is currently resolving. Application-scoping it + // (the original behavior) caused spurious "X -> X" self-loops when two requests near- + // simultaneously hit getInstance for the same controller on a cold framework (#2331); scoping + // it to the bare request scope then caused the same false positive between a request's own + // cfthreads, which share that scope on Lucee (#4448). Keying by thread id fixes both. var resolving = $getResolvingStack(); if (structKeyExists(resolving, arguments.name)) { throw( @@ -368,17 +366,50 @@ component implements="wheels.interfaces.di.InjectorInterface" { } /** - * Per-request resolving stack. Tracks which names are currently being - * resolved in THIS thread/request, so the circular-dependency guard - * doesn't see entries from concurrent requests. + * Per-thread resolving stack. Tracks which names the CURRENT thread is resolving, so the + * circular-dependency guard doesn't see entries from a concurrent request OR from a sibling + * cfthread of the same request. * - * Lazy-creates request.$wheelsDIResolving on first access. + * The outer struct is request-scoped — that is what keeps a cold-start race between two requests + * from reporting a false "X -> X" cycle (#2331). But on Lucee a `cfthread` shares its parent's + * request scope, so a single request's threads shared one stack: while thread A was inside a slow + * `init()` of service X, thread B's getInstance("X") saw X already present and threw a spurious + * Wheels.DI.CircularDependency (#4448). Keying the stack by thread id gives each thread its own set + * while keeping the request-scoped outer struct (and so #2331's semantics). + * + * The lazy creation of the shared outer struct and of each thread's sub-struct is locked so + * concurrent threads of one request don't race it (one overwriting the other's in-flight entries). */ private struct function $getResolvingStack() { - if (!structKeyExists(request, "$wheelsDIResolving")) { - request.$wheelsDIResolving = {}; + var tid = $resolvingThreadId(); + if (!StructKeyExists(request, "$wheelsDIResolving")) { + lock scope="request" type="exclusive" timeout="10" { + if (!StructKeyExists(request, "$wheelsDIResolving")) { + request.$wheelsDIResolving = {}; + } + } + } + if (!StructKeyExists(request.$wheelsDIResolving, tid)) { + lock scope="request" type="exclusive" timeout="10" { + if (!StructKeyExists(request.$wheelsDIResolving, tid)) { + request.$wheelsDIResolving[tid] = {}; + } + } + } + return request.$wheelsDIResolving[tid]; + } + + /** + * The current thread's id, or "0" on a JVM-free engine that has no java.lang.Thread (all such + * threads then share one key — the pre-#4448 behaviour, acceptable where cfthreads don't share a + * request scope anyway). Mirrors Migrator.$migrationLockThreadId(). + */ + private string function $resolvingThreadId() { + try { + return CreateObject("java", "java.lang.Thread").currentThread().getId(); + } catch (any e) { + return "0"; } - return request.$wheelsDIResolving; } /** diff --git a/vendor/wheels/tests/_assets/di/LatchedService.cfc b/vendor/wheels/tests/_assets/di/LatchedService.cfc new file mode 100644 index 0000000000..ca224fefe2 --- /dev/null +++ b/vendor/wheels/tests/_assets/di/LatchedService.cfc @@ -0,0 +1,28 @@ +/** + * Test fixture for #4448: a transient service whose init() deterministically holds the Injector's + * circular-dependency guard open for one thread while a second thread checks it. + * + * The FIRST thread to construct it signals `started` (it is now inside init(), so its name is in the + * resolving stack) and then blocks on `proceed`; the second thread waits for `started`, does its own + * getInstance (which is where the spurious CircularDependency used to throw), then releases `proceed`. + * Latches live in application scope so they are shared whether or not a cfthread shares the parent's + * request scope. Only the first construction coordinates — later ones return immediately — so the + * post-fix path (both threads construct) cannot deadlock. + */ +component { + + public any function init() { + if (StructKeyExists(application, "$di4448")) { + var state = {mine = application.$di4448.firstTaken.compareAndSet(JavaCast("boolean", false), JavaCast("boolean", true))}; + if (state.mine) { + application.$di4448.started.countDown(); + application.$di4448.proceed.await( + JavaCast("long", 10), + CreateObject("java", "java.util.concurrent.TimeUnit").SECONDS + ); + } + } + return this; + } + +} diff --git a/vendor/wheels/tests/specs/injector/InjectorCfthreadResolvingSpec.cfc b/vendor/wheels/tests/specs/injector/InjectorCfthreadResolvingSpec.cfc new file mode 100644 index 0000000000..5018753185 --- /dev/null +++ b/vendor/wheels/tests/specs/injector/InjectorCfthreadResolvingSpec.cfc @@ -0,0 +1,90 @@ +/** + * #4448: the Injector's circular-dependency guard was keyed on a request-scoped struct shared by a + * request's cfthreads (on Lucee a cfthread shares its parent's request scope). While one thread was + * inside a slow init() of transient service X — holding X in the resolving stack — a second thread's + * getInstance("X") saw X already present and threw a spurious Wheels.DI.CircularDependency. Keying the + * stack by thread id fixes it while keeping the request-scoped outer struct's #2331 semantics. + * + * This is deterministic (no timing sleeps): LatchedService.init() holds the FIRST thread inside init() + * via a latch until the second thread has gone through the guard. Threads are spawned from a component + * method, not an it() closure — cfthread can't be declared in a closure on Lucee. The latches are + * java.util.concurrent, so the spec skips on an engine without them (e.g. a JVM-free one). + */ +component extends="wheels.WheelsTest" { + + function run() { + + describe("Injector circular-dependency guard across a request's cfthreads (4448)", () => { + + it("does not throw a spurious CircularDependency when two threads resolve the same transient at once", () => { + if (!$di4448Supported()) { + skip("requires java.util.concurrent latches (JVM engine)"); + } + var results = $runDiConcurrencyProbe(); + expect(results.t1).toBe("ok"); + expect(results.t2).toBe("ok"); + }); + + }); + + } + + private boolean function $di4448Supported() { + try { + CreateObject("java", "java.util.concurrent.CountDownLatch").init(JavaCast("int", 1)); + return true; + } catch (any e) { + return false; + } + } + + /** + * Spawn two cfthreads that resolve the same transient service concurrently, overlapped so thread 2 + * passes the guard while thread 1 is inside the service's init(). Returns both thread results. + */ + private struct function $runDiConcurrencyProbe() { + application.$di4448 = { + started = CreateObject("java", "java.util.concurrent.CountDownLatch").init(JavaCast("int", 1)), + proceed = CreateObject("java", "java.util.concurrent.CountDownLatch").init(JavaCast("int", 1)), + firstTaken = CreateObject("java", "java.util.concurrent.atomic.AtomicBoolean").init(JavaCast("boolean", false)) + }; + var di = new wheels.Injector(binderPath = "wheels.tests._assets.di.TestBindings"); + di.map("latched4448").to("wheels.tests._assets.di.LatchedService"); + application.$di4448Injector = di; + + try { + thread name="di4448_t1" action="run" { + thread.result = "unset"; + try { + application.$di4448Injector.getInstance("latched4448"); + thread.result = "ok"; + } catch (any e) { + thread.result = e.type; + } + } + thread name="di4448_t2" action="run" { + thread.result = "unset"; + try { + // Wait until t1 is inside init() (holding the guard), then resolve. + application.$di4448.started.await( + JavaCast("long", 10), + CreateObject("java", "java.util.concurrent.TimeUnit").SECONDS + ); + application.$di4448Injector.getInstance("latched4448"); + thread.result = "ok"; + } catch (any e) { + thread.result = e.type; + } finally { + // Release t1 whether t2 passed or threw, so t1 always finishes. + application.$di4448.proceed.countDown(); + } + } + thread action="join" name="di4448_t1,di4448_t2" timeout="20000"; + return {t1 = cfthread.di4448_t1.result, t2 = cfthread.di4448_t2.result}; + } finally { + StructDelete(application, "$di4448"); + StructDelete(application, "$di4448Injector"); + } + } + +}