servlet: disable RECYCLE_FACADES to reduce flaky tests #12530
Conversation
becomeStar
force-pushed
the
servlet/fix-flaky-recycle
branch
from
67c9aa0 to
120a197
Compare
|
I re-read the contribution guidelines and realized I had incorrectly included two lines of code formatting along with the core logic change, which violates the "Don't fix code style" rule. I have reverted the formatting changes, so this PR now only contains the intended one line of core logic change: .setDiscardFacades(false);. I believe this single line resolves the issue described in #12524, but I would like to confirm if the maintainers also consider this approach appropriate. |
Set discardFacades=false in Tomcat 10 Embedded to avoid premature OutputBuffer recycling. This prevents flaky tests in gRPC servlet transport by ensuring facades are not discarded too early. Fixes grpc#12524
becomeStar
force-pushed
the
servlet/fix-flaky-recycle
branch
from
120a197 to
7272a28
Compare
Why are they prematurely recycled? That seems to be the problem. It is either a bug in Tomcat or in gRPC. The linked issue says:
So it seems we need to fix our code to stop using the request when it is no longer valid. Or at least that should be the initial goal. Maybe we can't because of some reason, but then we'd want to understand why. |
|
I would accept this change as a "make the tests stop flaking," but then we'd want to create a new issue to track the bug involved here. |
|
Thank you for the insightful feedback. You are absolutely right. I agree that disabling facade recycling is a workaround to stabilize the tests. We should investigate whether gRPC is accessing the request/response objects (specifically the underlying OutputBuffer) after their lifecycle has ended. As you suggested, I will:
I'll create the issue shortly and reference it here. |
|
I've created the tracking issue: #12540. |
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
One small change, then seems it could go in.
| .setAsyncSupported(true); | ||
| ctx.addServletMappingDecoded("/*", "TomcatTransportTest"); | ||
| tomcatServer.getConnector().addUpgradeProtocol(new Http2Protocol()); | ||
| tomcatServer.getConnector().setDiscardFacades(false); |
There was a problem hiding this comment.
Let's add a comment saying this is a workaround and have a link to the issue you created.
ejona86
added
the
kokoro:run
grpc-kokoro
removed
the
kokoro:run
becomeStar
force-pushed
the
servlet/fix-flaky-recycle
branch
from
cde3d48 to
7272a28
Compare
ejona86
added
the
kokoro:run
grpc-kokoro
removed
the
kokoro:run
3 participants
Background
In the gRPC servlet transport with Tomcat 10 Embedded,
we observed occasional flaky tests in
grpc-servlet-jakarta:tomcat10Test.The issue is related to a race condition where the OutputBuffer is prematurely recycled during asynchronous writes.
Changes
Reference: spring-projects/spring-boot#36763 (comment)
This PR explicitly calls
.setDiscardFacades(false)on the Tomcat 10 Embedded Connector.The intention is to prevent outputBuffer reuse, so that the above race condition does not occur during tests.
Purpose
grpc-servlet-jakarta:tomcat10TestNote
This change aligns Tomcat 10 Embedded behavior with Tomcat 9 (where
RECYCLE_FACADESdefaults tofalse), and is known to prevent prematureOutputBufferrecycling during async writes.Based on the investigation and prior reports (e.g., Spring Boot discussion),
this is currently the correct and safe configuration for servlet-based asynchronous responses in Tomcat 10.
Fixes #12524