CAMEL-24811: camel-servlet - fix async servlet completing before the route finishes - #26584
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 72 tested, 27 compile-only — current: 75 all testedMaveniverse Scalpel detected 99 affected modules (current approach: 75).
|
| Module | Duration | Status |
|---|---|---|
| Camel :: Jetty | 47.5s | SUCCESS |
| Camel :: Servlet | 33.2s | SUCCESS |
| Camel :: HTTP :: Common | 8.6s | SUCCESS |
Top 20 slowest modules:
Camel :: Jetty(47.5s)Camel :: Servlet(33.2s)Camel :: HTTP :: Common(8.6s)
04a6472 to
eb6bd33
Compare
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Reviewed the async completion fix carefully — it's correct, and the test pins the exact race.
The bug: doServiceAsync() (async=true, no executorRef) called AsyncContext.complete() unconditionally after doService() returned, discarding the CompletionStage from doExecute()/processAsync(). So when the route genuinely completes on another thread, the servlet container's async cycle was completed before the route wrote the response — a race.
The fix is sound:
doService()isvoid(subclasses override it), so the in-flightCompletionStageis handed over through theCamelAsyncPromiserequest attribute, anddoServiceAsync()retrieves it and defers completion viacompleteOnCompletion().completeOnCompletion()completes the context in both cases — immediately when there is no promise (synchronous), and viapromise.whenComplete((r, e) -> context.complete())when in-flight.whenCompleteon an already-completed stage still runs, so there is no lost-completion window; the attribute is set and read on the same servlet thread, so no visibility race.- Exactly one
complete()per request: the success path goes throughcompleteOnCompletionand the exception path through thecatch/finally, and they are mutually exclusive — no double-complete. - Two related correctness fixes come along: a synchronous
processAsyncfailure now setsisAsync=falseso the error response is written synchronously, andtryAsyncProcess'swhenCompletenow always callsafterProcess(writing the response and finishing the unit of work even when the stage completed exceptionally, which the old success-only branch skipped).
Test: ServletAsyncNoExecutorRefRaceTest#testAsyncRouteCompletesBeforeResponse uses an asyncDelayed route that resumes on the delayer's scheduler thread and asserts the response is 200 with body delayed-response — which fails on the premature-completion race and passes with the fix. JettyAsyncDelayedRouteTest covers the jetty side.
Minor (non-blocking): the new servlet test uses JUnit assertEquals; AssertJ is the project preference for new test code.
CI is green — approving.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of oscerd
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Nice fix, and a thorough write-up — the analysis matches the history: the discarded CompletionStage in doServiceAsync() goes back to CAMEL-11731 (Camel 3.7), which fixed the executorRef branch but not the context.start() branch.
Verified locally on the PR branch (not just from the diff):
- Full
camel-servletsuite: 100 run, 0 failures.camel-jetty*Async*,*Continuation*,JettyRouteTest,HttpRouteTest+JettyAsyncDelayedRouteTest: 31 run, 0 failures. - Swapped in
main'sCamelServlet.javaand re-ranServletAsyncNoExecutorRefRaceTest: it fails withexpected: <delayed-response> but was: <>, so the new test genuinely reproduces the race. - Only in-tree
doService()overrides areCamelContinuationServlet(delegates tosuper.doService()foruseContinuation=false, which is exactly why the request-attribute hand-over is needed) andCamelWebSocketServlet(never calls super, so the attribute stays null and behaviour is unchanged). Nobody overridesdoServiceAsync(). KeepingdoService()voidfor binary compatibility is the right call for the LTS backports. - The
whenComplete(context.complete())is chained on thewhenComplete(afterProcess)stage, so the response is written before theAsyncContextcompletes;isAsync = falsein the catch and the always-afterProcess()intryAsyncProcess()close the leaked-UoW/unwritten-response gaps without any doubleafterProcess(). Thefinally { context.complete(); }additions are needed sinceonError()always throws.
One small non-blocking question inline about the visibility of the new constant.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of davsclaus
| * {@link #doService(HttpServletRequest, HttpServletResponse)} returns. The {@link AsyncContext} must not be | ||
| * completed before it. | ||
| */ | ||
| public static final String ASYNC_PROMISE_ATTRIBUTE_NAME = "CamelAsyncPromise"; |
There was a problem hiding this comment.
Non-blocking: only CamelServlet itself reads and writes this attribute. Since doService() is a protected extension point, public is defensible so a subclass that fully replaces doService() can hand over its own stage — but if that is not the intent, protected would keep it off the public API surface. Either is fine with me; just flagging it since it is new public API on a class that gets backported.
eb6bd33 to
c39a0cf
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-reviewing after the new push (c39a0cf).
Previous findings addressed: The ASYNC_PROMISE_ATTRIBUTE_NAME visibility question raised inline by @davsclaus is answered: the PR keeps it as protected static final, which is the right call — it's readable by subclasses that need to hand over their own stage via doService(), but it stays off the broader public API surface.
All three correctness fixes (race in doServiceAsync, finally-block in doAsyncExecution, always-afterProcess in tryAsyncProcess) look correct. No double-afterProcess, no lost-completion window, no visibility race on the attribute.
One gap: the tryAsyncProcess().whenComplete() fix — always calling afterProcess even when the stage completes exceptionally — has no test. Both new tests only exercise the success path (route delays then returns a body). A test where the async route throws (or the processor stage completes with an exception) would verify: (a) an error response is written rather than an empty/stale body, and (b) the AsyncContext is completed exactly once. Without it, a regression to the old success-only branch would not be caught.
Suggested addition to ServletAsyncNoExecutorRefRaceTest (or a companion ServletAsyncErrorTest):
// Route that resumes on another thread and then throws
from("servlet:async-error?async=true")
.delay(100).asyncDelayed().end()
.process(x -> { throw new RuntimeException("boom"); });
// Test: expect 500, not a hung request or empty body
WebResponse resp = query(new GetMethodWebRequest(contextUrl + "/services/async-error"), false);
assertEquals(500, resp.getResponseCode());This review was generated by an AI agent, Hermès on behalf of @gnodet.
| try { | ||
| afterProcess(res, consumer, exchange, false); | ||
| } catch (Exception e) { | ||
| exchange.setException(e); |
There was a problem hiding this comment.
afterProcess() outside the else branch so it runs even when the stage completes with an exception — that's the right fix. But neither new test exercises this path: both ServletAsyncNoExecutorRefRaceTest and JettyAsyncDelayedRouteTest only verify the success case (route delays, returns body). A test where processAsync() completes exceptionally would confirm that (a) an error response is actually written and (b) the AsyncContext is completed exactly once, preventing a silent regression back to the old success-only branch.
…route finishes CamelServlet.doServiceAsync() (async=true without executorRef) completed the AsyncContext right after doService() returned, discarding the CompletionStage that doExecute() returns when the route continues on another thread. This races the container's async completion against the later writeResponse(), giving an IllegalStateException on the recycled response (Tomcat) or an empty/stale response. camel-jetty in async mode (async=true&useContinuation=false) hits the same path via super.doService(). doService() keeps its void signature (protected extension point, must stay binary compatible for backports); the in-flight stage is handed over via the CamelAsyncPromise request attribute and doServiceAsync() completes the AsyncContext only once it is done, mirroring doAsyncExecution(). Also: complete the AsyncContext in doAsyncExecution()'s error branch (onError always throws), and always write the response / finish the UoW when processAsync() fails synchronously or completes exceptionally. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
c39a0cf to
5000834
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-reviewing after the new push (5000834).
Previous finding addressed: ServletAsyncErrorTest is exactly what was requested — testAsyncRouteThrows covers the path where an async route resumes on another thread and then throws (verifying afterProcess is called and a 500 is written), and testAsyncStageCompletesExceptionally covers the case where the processAsync() stage itself completes exceptionally (verifying the tryAsyncProcess().whenComplete() fix actually writes the error response rather than silently dropping it).
All four correctness fixes are intact and covered:
doServiceAsync()race —AsyncContext.complete()now chained onto the in-flight stage via the request attribute hand-offdoAsyncExecution()error branch —context.complete()guaranteed infinallytryAsyncProcess()exceptional stage —afterProcess()always called, not only on the happy pathdoExecute()synchronousprocessAsync()exception —isAsync = falseensuresafterProcess()is called on the calling thread
Approved.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…route finishes (backport 4.22.x) Backport of #26584 to camel-4.22.x (straight cherry-pick, no conflicts). CamelServlet.doServiceAsync() (async=true without executorRef, and camel-jetty with async=true&useContinuation=false) completed the AsyncContext before a route that resumes on another thread had written its response. The bug dates from CAMEL-11731 (Camel 3.7), so every LTS branch is affected. No API changes: doService() keeps its signature, the in-flight stage is handed over via a protected request attribute constant. Tests cover the no-executorRef race in camel-servlet and the delayed route in camel-jetty. Closes #26596 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 participants
Summary
CamelServlet.doServiceAsync()(used whenasync=trueand noexecutorRefis configured — the simplest, most common async servlet setup) calledAsyncContext.complete()unconditionally right afterdoService()returned, discarding theCompletionStagethatdoExecute()returns when the route's processor genuinely completes on another thread (AsyncProcessor.processAsync()).This races the servlet container's async completion against the later
afterProcess()/writeResponse()call:IllegalStateException: The response object has been recycled and is no longer associated with this facade.The same applies to
camel-jettyin async mode (jetty:...?async=true&useContinuation=false):CamelContinuationServlet.handleDoService()falls back tosuper.doService()there and ends up in the samedoServiceAsync()path.The sibling path used when
executorRefis configured (doAsyncExecution()) already does this correctly — it chainsAsyncContext.complete()ontopromise.whenComplete(...). This bug predates 4.21/4.22: it was introduced by CAMEL-11731 (Camel 3.7) when true async processing was added todoExecute(), without updating the no-executorRefdoServiceAsync()dispatch path to match. It therefore affects every current LTS branch.Changes
doService()keeps itsvoidsignature (it is aprotectedextension point overridden byCamelContinuationServlet,CamelWebSocketServletand third-party servlets, so changing its return type would be a source- and binary-incompatible change and not backportable). Instead, whendoExecute()returns a still in-flightCompletionStage,doService()stores it in the request attributeCamelServlet.ASYNC_PROMISE_ATTRIBUTE_NAME.doServiceAsync()reads that attribute afterdoService()returns and completes theAsyncContextonly once the stage completes, mirroringdoAsyncExecution(). Because the hand-over is via the request rather than the return value, it also works when a subclass (jetty) delegates tosuper.doService()and drops the result.doAsyncExecution()'s error branch now completes theAsyncContextin afinally:onError()always throws, so previously a synchronous exception there never completed the context and the request only ended by async timeout.doExecute()/tryAsyncProcess(), both of which previously left the response unwritten and the UoW unfinished: a synchronous exception fromprocessAsync()now falls back to the synchronousafterProcess(), and an exceptionally completed stage now still callsafterProcess()(which writes the error response and finishes the UoW).ServletAsyncNoExecutorRefRaceTest(camel-servlet, embedded Undertow) andJettyAsyncDelayedRouteTest(camel-jetty,async=true&useContinuation=false) reproducing the bug with a genuinely-async route step (delay(...).asyncDelayed()), independent of any external service.ServletAsyncErrorTestcovering the error path: a route that fails after resuming on another thread, and a consumer whoseprocessAsync()stage completes exceptionally (a route processor never does that — its exception lands on the exchange and the stage completes normally — so this uses a customAsyncProcessor). Both expect a 500; with thetryAsyncProcess()hunk reverted, the second one gets an empty 200.No user-facing API changes, so no upgrade guide entry; the fix is a straight cherry-pick candidate for the LTS branches.
Test plan
ServletAsyncNoExecutorRefRaceTestandJettyAsyncDelayedRouteTestfail before the fix (HTTP 200 with an empty body) and pass afterServletAsyncErrorTest.testAsyncStageCompletesExceptionallyfails (HTTP 200) with thetryAsyncProcess()change reverted, passes with itcamel-servletfull test suite passescamel-jetty*Async*,*Continuation*,JettyRouteTest,HttpRouteTestpasscamel-jetty-commonandcamel-atmosphere-websocketcompile unchangedFixes CAMEL-24811.
Claude Code on behalf of Croway