Skip to content

Commit the response on a swallowed Throwable - #9

Open
mnjustin wants to merge 1 commit into
foundeo:masterfrom
mnjustin:fix-swallowed-throwable-hangs-latch
Open

Commit the response on a swallowed Throwable#9
mnjustin wants to merge 1 commit into
foundeo:masterfrom
mnjustin:fix-swallowed-throwable-hangs-latch

Conversation

@mnjustin

Copy link
Copy Markdown

A Throwable from service() is logged and swallowed, leaving the response uncommitted. Because AwsHttpServletResponse.flushBuffer() is the only caller of countDown() on the latch that LambdaContainerHandler.proxy() waits on, nothing releases that latch and the invocation hangs until the Lambda runtime interrupts it — surfacing as an InterruptedException at CountDownLatch.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:

if (!response.isCommitted() && request.getDispatcherType() != DispatcherType.ASYNC) {
    response.flushBuffer();
}

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 in finally rather than after service() 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 finally guard, a thrown exception produces a committed response that never had a status set. AwsHttpServletResponse.getStatus() returns:

return (statusCode <= 0 ? SC_OK : statusCode);

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 through flushBuffer(), so it releases the latch on its own as well.

3. Rethrow Error.

An OutOfMemoryError or StackOverflowError swallowed by catch (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 Error rethrow ordering. The finally still runs before the rethrown Error propagates, so flushBuffer() 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 the Error and recycle the container — but the alternative is to skip the flush entirely for an Error and 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()'s statusCode <= 0 fallback as meaning an uncommitted, unset response goes out as 200, but I have only traced it, not reproduced it deliberately.

Notes

Branched from 747a327 rather than from my fork's master, 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 archivesBaseName property in java/build.gradle before 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant