diff --git a/system/web/services/InterceptorService.cfc b/system/web/services/InterceptorService.cfc index c24a98692..28318c7af 100644 --- a/system/web/services/InterceptorService.cfc +++ b/system/web/services/InterceptorService.cfc @@ -218,7 +218,15 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { var event = controller.getRequestService().getContext() // Threaded paths hand the buffer to a background thread that can outlive this call, so // it can never be safely pooled/reused - only the synchronous (default) path pools it. - var pooled = !arguments.async && !arguments.asyncAll + // Also never pool while already executing inside a cfthread (e.g. a synchronous announce() + // triggered from code running inside an async/asyncAll thread, such as getInstance()'s + // afterInstanceAutowire announcement): `request` scope - and thus the pool array - is + // shared between the request thread and any cfthread spawned from it, and CFML/BoxLang + // arrays aren't thread-safe, so two real concurrent threads popping/releasing the same + // array can corrupt it ("can not pop Element from array, array is empty" - COLDBOX-1454). + // InterceptorState.process() uses the same controller.getUtil().inThread() check for its + // own async dispatch decision. + var pooled = !arguments.async && !arguments.asyncAll && !variables.controller.getUtil().inThread() var buffer = getLazyBuffer( pooled ) try { @@ -259,23 +267,37 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { * instead of allocating a new component every time. A reentrant announce() (e.g. an * interceptor that itself triggers `announce( "onException", ... )` while executing) finds * the pool empty and gets its own instance, so it can never clobber the in-flight buffer of - * the call still running further up the stack. The async/asyncAll paths always get a fresh, - * unpooled instance since their buffer is handed to a background thread that can outlive - * this call. + * the call still running further up the stack. The async/asyncAll paths, and any announce() + * running inside a cfthread, always get a fresh, unpooled instance since their buffer can be + * used by a background thread that can outlive this call. + * + * The pool lives in `request` scope, but an async announce()'s thread can itself outlive the + * request that spawned it (a fire-and-forget async announce, or one nobody joined) - by the + * time that orphaned thread's own work runs, `request` scope may no longer be the one this + * pool was built against, or may not be usable at all. The pool is a performance nicety only, + * never load-bearing for correctness, so any failure reading it here is treated the same as + * "no pool available" - fall back to a fresh, unpooled buffer rather than letting the + * announce() itself fail. * * @pooled Whether to check out from (and later return to) the per-request pool * * @return { get(), clear(), append(), length(), getString() } */ function getLazyBuffer( boolean pooled = true ){ - if ( - arguments.pooled && - structKeyExists( request, "cbox_interceptorBufferPool" ) && - request.cbox_interceptorBufferPool.len() - ) { - var buffer = request.cbox_interceptorBufferPool.pop() - buffer.clear() - return buffer + if ( arguments.pooled ) { + try { + if ( + structKeyExists( request, "cbox_interceptorBufferPool" ) && + request.cbox_interceptorBufferPool.len() + ) { + var buffer = request.cbox_interceptorBufferPool.pop() + buffer.clear() + return buffer + } + } catch ( any e ) { + // request scope is gone or unusable (e.g. this thread has outlived the request + // that spawned it) - the pool goes with it, fall through to an unpooled buffer. + } } return new coldbox.system.web.context.InterceptorBuffer() } @@ -283,13 +305,20 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { /** * Return a buffer checked out via getLazyBuffer( true ) back to the per-request pool. * + * Silently drops the buffer if `request` scope is gone or unusable (see getLazyBuffer()) - + * there is no pool left to release it into, and that's fine, it just won't be reused. + * * @buffer The buffer instance to release */ function releaseLazyBuffer( required buffer ){ - if ( !structKeyExists( request, "cbox_interceptorBufferPool" ) ) { - request.cbox_interceptorBufferPool = [] + try { + if ( !structKeyExists( request, "cbox_interceptorBufferPool" ) ) { + request.cbox_interceptorBufferPool = [] + } + request.cbox_interceptorBufferPool.append( arguments.buffer ) + } catch ( any e ) { + // Nothing to release into - safe to drop. } - request.cbox_interceptorBufferPool.append( arguments.buffer ) } /** diff --git a/tests/specs/web/services/InterceptorserviceTest.cfc b/tests/specs/web/services/InterceptorserviceTest.cfc index 374b5bf0d..5b77e23b8 100755 --- a/tests/specs/web/services/InterceptorserviceTest.cfc +++ b/tests/specs/web/services/InterceptorserviceTest.cfc @@ -3,6 +3,12 @@ function setup(){ super.setup(); + // `request` scope persists across every test in this suite (they all run inside one + // physical HTTP request to the test runner), so a leftover interceptor buffer pool - or + // one a test deliberately broke to exercise the COLDBOX-1454 fallback - must not leak + // into the next test. + structDelete( request, "cbox_interceptorBufferPool" ); + // Create Mock Objects variables.mockbox = getMockBox(); variables.mockController = mockBox.createMock( "coldbox.system.testing.mock.web.MockController" ); @@ -23,6 +29,11 @@ mockController.setLogBox( mockLogBox ); mockController.setWireBox( mockWireBox ); mockController.setCacheBox( mockCacheBox ); + // A real setter, not a $() stub: MockBox's $() return-type inference for getUtil() + // misfires on Lucee 5/6 (it infers "string" and then fails to cast the real Util + // component to it), the same way setLogBox()/setWireBox()/setCacheBox() above sidestep + // it for their own properties. + mockController.setUtil( new coldbox.system.core.util.Util() ); mockRequestService.$( "getFlashScope", mockFlash ); mockLogBox.$( "getLogger", mockLogger ); @@ -177,6 +188,88 @@ expect( local.output ).toBe( "buffered output" ) } + /** + * Unambiguous reference-identity check: CFML's own object equality can compare by value/ + * string representation, which makes two freshly-created, still-empty InterceptorBuffer + * instances look "equal" even though they're genuinely different objects. The JVM's identity + * hash sidesteps that entirely. + */ + private function objectId( required obj ){ + return createObject( "java", "java.lang.System" ).identityHashCode( arguments.obj ) + } + + function testAnnouncePoolsAndReusesTheBufferWhenNotInsideAThread(){ + var buffers = [] + iService.listen( function( event, data, buffer ){ + buffers.append( arguments.buffer ) + }, "onPoolTest" ) + + iService.announce( "onPoolTest" ) + iService.announce( "onPoolTest" ) + + // Sequential, non-threaded announce() calls reuse the one pooled buffer + expect( objectId( buffers[ 1 ] ) ).toBe( objectId( buffers[ 2 ] ) ) + } + + function testAnnounceNeverPoolsTheBufferWhileInsideAThread(){ + // COLDBOX-1454: `request` scope - and thus the buffer pool array - is shared between + // the request thread and any cfthread spawned from it. A synchronous announce() running + // on a spawned thread (e.g. WireBox's afterInstanceAutowire, triggered by getInstance() + // from code running inside an async/asyncAll announce()'s thread) must never touch the + // same pool the request thread is using, or two real concurrent threads can pop/release + // the same array at once and corrupt it. + mockController.setUtil( mockBox.createStub().$( "inThread", true ) ) + + var buffers = [] + iService.listen( function( event, data, buffer ){ + buffers.append( arguments.buffer ) + }, "onPoolTest" ) + + iService.announce( "onPoolTest" ) + iService.announce( "onPoolTest" ) + + expect( objectId( buffers[ 1 ] ) ).notToBe( objectId( buffers[ 2 ] ) ) + } + + function testGetLazyBufferPoolsAndReusesWhenAskedTo(){ + var buffer1 = iService.getLazyBuffer( true ) + iService.releaseLazyBuffer( buffer1 ) + var buffer2 = iService.getLazyBuffer( true ) + + expect( objectId( buffer1 ) ).toBe( objectId( buffer2 ) ) + } + + function testGetLazyBufferNeverReusesWhenNotAskedTo(){ + var buffer1 = iService.getLazyBuffer( false ) + var buffer2 = iService.getLazyBuffer( false ) + + expect( objectId( buffer1 ) ).notToBe( objectId( buffer2 ) ) + } + + function testGetLazyBufferFallsBackToAnUnpooledBufferWhenThePoolIsUnusable(){ + // COLDBOX-1454: an async announce()'s thread can outlive the request that spawned it (a + // fire-and-forget async announce, or one nobody joined). By the time that orphaned + // thread's own work runs, `request` scope - and the pool living in it - may no longer be + // usable. The pool is a performance nicety only, so this must degrade to a fresh, + // unpooled buffer instead of throwing. A struct in place of the expected array simulates + // that unusable state: structs have no pop() method, so reading it throws. + request.cbox_interceptorBufferPool = { "not" : "an array" } + + var buffer = iService.getLazyBuffer( true ) + + expect( isObject( buffer ) ).toBeTrue() + expect( buffer.hasContent() ).toBeFalse() + } + + function testReleaseLazyBufferSilentlyDropsWhenThePoolIsUnusable(){ + // Same COLDBOX-1454 scenario as above, from the release side: a plain string in place of + // the expected array has no append() method, so writing back to it throws - and that + // must not propagate out of announce()'s finally block. + request.cbox_interceptorBufferPool = "not an array" + + iService.releaseLazyBuffer( new coldbox.system.web.context.InterceptorBuffer() ) + } + function testInterceptionPoints(){ // test registration again assertTrue( arrayLen( iService.getInterceptionPoints() ) gt 0 );