COLDBOX-1454: fix interceptor buffer pool race between a request and its async announce thread - #705
Conversation
… and its async announce thread InterceptorService.getLazyBuffer()'s per-request buffer pool only skipped pooling for the announce() call that itself spawns an async/ asyncAll thread. It missed that code running *inside* that spawned thread can make its own synchronous announce() calls - most commonly WireBox's afterInstanceAutowire announcement, fired by every getInstance() call - which still pool against the same request-scoped array as the original request thread. `request` scope, and thus the pool array, is shared between a request and any cfthread spawned from it. CFML/BoxLang arrays aren't thread-safe, so two real concurrent threads (the request thread and an async announce()'s thread) popping/releasing the same array can corrupt it, throwing "can not pop Element from array, array is empty" mid-request. Also check controller.getUtil().inThread() (the same check InterceptorState.process() already uses for its own async dispatch decision) so a synchronous announce() executing inside any cfthread always gets a fresh, unpooled buffer - never touching the array the request thread is using. Added regression coverage for both the pooled (reused) and never-pooled (inside a thread / not asked to pool) paths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KiK6DMek9iJMuYk2PzcjPj
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The regression test stubs inThread() instead of exercising an actual joined spawned thread.
Review effort: Lite
Findings: None
What changed in this PR
Fixes a race in request-scoped interceptor buffer pooling when announcements execute in spawned threads.
Changes:
- Disables pooling inside threads.
- Adds regression tests for pooled and unpooled buffer behavior.
| File | Summary |
|---|---|
tests/specs/web/services/InterceptorserviceTest.cfc |
Adds buffer pooling regression coverage. |
system/web/services/InterceptorService.cfc |
Prevents shared pool access from spawned threads. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
An async announce()'s thread can itself become unbound from the request that spawned it - a fire-and-forget async announce, or one nobody joined, can keep running after the request has already ended. By the time that orphaned thread's own work touches the buffer pool, `request` scope may no longer be the one the pool was built against, or may not be usable at all. The pool is a performance nicety, never load-bearing for correctness, so getLazyBuffer()/releaseLazyBuffer() now treat any failure reading or writing it the same as "no pool available": fall back to a fresh, unpooled buffer (get) or silently drop it (release), rather than letting that propagate out of announce(). Added regression tests for both directions, and hardened the test file's setup() to clear any leftover pool state between tests, since request scope - and thus the pool - persists across every test in a TestBox run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KiK6DMek9iJMuYk2PzcjPj
… in tests CI showed 16 errors on lucee@5/6, all in InterceptorserviceTest.cfc: mockController.$( "getUtil", ... ) made MockBox generate a stub method whose inferred return type was "string", then fail to cast the real Util component (or, in one test, a TestBox Stub) to it. Switched to mockController.setUtil( ... ), a real accessor-generated setter rather than a MockBox $() stub - the same pattern setLogBox()/ setWireBox()/setCacheBox() already use for their own properties in this same setup(), and which doesn't go through MockBox's return-type inference at all. Lucee 5/6 aren't available in this sandbox (no cached artifact, and ForgeBox is unreachable here) to reproduce directly, but this removes the exact code path CI's stack trace pointed at, in favor of a pattern already proven safe on every engine in this same file. Re-verified 27/27 passing on Lucee 7 across repeated runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KiK6DMek9iJMuYk2PzcjPj
|
The base I don't have permission to re-run the failed job from here ( Generated by Claude Code |
Description
Fixes COLDBOX-1454:
InterceptorService.getLazyBuffer()'s per-request buffer pool (added in #701 as a performance optimization) races between a request thread and its ownannounce( ..., async: true )thread, throwingcan not pop Element from array, array is emptyin production.Root cause
getLazyBuffer()'s pooling decision only excluded theannounce()call that itself spawns an async/asyncAll thread. It missed that code running inside that spawned thread can make its own synchronousannounce()calls — most commonly WireBox'safterInstanceAutowireannouncement, fired on everygetInstance()call — and those still pooled against the samerequest-scoped array as the original request thread.requestscope — and thus the pool array — is shared between a request and anycfthreadspawned from it. CFML/BoxLang arrays aren't thread-safe, so two real concurrent threads (the request thread and an asyncannounce()'s thread) popping/releasing the same array at once can corrupt it.Fix
Also check
controller.getUtil().inThread()— the same checkInterceptorState.process()already uses for its own async dispatch decision — so a synchronousannounce()executing inside any cfthread always gets a fresh, unpooled buffer, never touching the array the request thread is using:This matches the fix suggested in the ticket.
Validation
InterceptorserviceTest.cfcfor both the pooled/reused path and the never-pooled paths (async/asyncAll, and now inside-a-thread) — 25/25 passing locally against a live BoxLang/Lucee stack (bypassing this sandbox's ORM/MySQL-dependent shared test harness by running the spec directly via TestBox).InterceptorStateTest.cfcsuite (16/16 passing) to confirm no collateral effects, since it shares the sameutility.inThread()pattern.cfthreads hammeringgetInstance()on both the main and spawned threads; it did not reproduce in this sandbox (Lucee 7 here vs. Lucee 6.2.7 in the report — possibly a more forgiving array implementation, or just scheduling luck). The fix is still correct by construction: it removes cross-thread access to the shared pool array entirely rather than reducing collision odds, and it matches the reporter's own root-cause diagnosis and suggested fix.Type of change
Checklist
🤖 Generated with Claude Code
https://claude.ai/code/session_01KiK6DMek9iJMuYk2PzcjPj
Generated by Claude Code