Commit the response on a swallowed Throwable - #9
Open
mnjustin wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A
Throwablefromservice()is logged and swallowed, leaving the response uncommitted. BecauseAwsHttpServletResponse.flushBuffer()is the only caller ofcountDown()on the latch thatLambdaContainerHandler.proxy()waits on, nothing releases that latch and the invocation hangs until the Lambda runtime interrupts it — surfacing as anInterruptedExceptionatCountDownLatch.await()and a 5xx to the client.This came out of chasing an unrelated intermittent 503 on a FuseLess-on-Lambda deployment. That fault turned out to be Lucee's background controller thread calling
Thread.interrupt(), not this — but this path was found along the way and produces the same symptom whenever Lucee does throw here.The three changes
1. Commit the response in
finally.AwsLambdaServletContainerHandler.doFilter()ends with exactly this guard:FuseLess calls
service()directly rather than going through the filter chain — a reasonable choice, since a CFML container has no servlet filters to apply — so that guard is never reached. This replicates it. It is infinallyrather than afterservice()so it also covers the catch path.2. Send a 500 on the catch path.
This is the part that surprised me. With only the
finallyguard, a thrown exception produces a committed response that never had a status set.AwsHttpServletResponse.getStatus()returns:So a servlet that throws before setting a status is reported to the client as HTTP 200 with an empty body. A failed request looks like a successful one, both to the caller and to any monitoring keyed on status codes — arguably worse than the hang, which at least fails visibly.
sendError()routes throughflushBuffer(), so it releases the latch on its own as well.3. Rethrow
Error.An
OutOfMemoryErrororStackOverflowErrorswallowed bycatch (Throwable)leaves the container in an unknown state while reporting a normal response. Letting it propagate lets the runtime fail the invocation and replace the execution environment.Questions
On the
Errorrethrow ordering. Thefinallystill runs before the rethrownErrorpropagates, soflushBuffer()executes on the OOM path too. That seemed right to me — release the latch so the client gets a response, then let the runtime see theErrorand recycle the container — but the alternative is to skip the flush entirely for anErrorand let the invocation fail hard rather than responding from a JVM in an unknown state. Happy to change it if you would rather it not respond at all in that case.On placement. I have put all of this in
handleRequest. If you would rather the commit guarantee live somewhere closer to the library's own structure, or handled differently given FuseLess deliberately skips the filter chain, I am glad to move it.On the 200. Worth a sanity check from someone who knows this code better — I read
getStatus()'sstatusCode <= 0fallback as meaning an uncommitted, unset response goes out as 200, but I have only traced it, not reproduced it deliberately.Notes
Branched from
747a327rather than from my fork'smaster, so this contains only this change and none of the Lucee 7 / jakarta work in my other PRs.Not compile-verified locally. This branch predates the Gradle upgrade, and Gradle 9 rejects the
archivesBaseNameproperty injava/build.gradlebefore it gets as far as compiling. The failure is identical with these changes stashed, so it is pre-existing rather than introduced here — but I have not been able to build it, and CI or your own build would be the check.