servlet: fix write when not ready in AsyncServletOutputStreamWriter by mgustimz · Pull Request #12790 · grpc/grpc-java

mgustimz · 2026-05-02T16:33:35Z

Description

Even when isReady() returns true, the servlet container may still be processing a previous write internally. Always set readyAndDrained to false after a direct write in runOrBuffer() to ensure subsequent writes go through the container thread via onWritePossible(), matching the servlet spec requirement.

Tomcat throws:

java.lang.IllegalStateException: In non-blocking mode you may not write to the ServletOutputStream until the previous write has completed and isReady() returns true

Testing

Validated by running servlet tests locally. TomcatTransportTest.clientChecksInboundMetadataSize_header passes.

Fixes

Fixes #12723

Fixes grpc#12723

Copilot

Pull request overview

This PR aims to address Tomcat's non-blocking write enforcement in the servlet transport by changing how AsyncServletOutputStreamWriter tracks readiness after direct application-thread writes.

Changes:

Updates runOrBuffer() so direct non-complete actions always clear the readyAndDrained state.
Intended to route subsequent writes back through onWritePossible() instead of continuing direct writes from the application thread.
Targets the servlet transport path implicated by TomcatTransportTest.clientChecksInboundMetadataSize_header.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+      boolean successful =
+          writeState.compareAndSet(curState, curState.withReadyAndDrained(false));
+      LockSupport.unpark(parkingThread);
+      checkState(successful, "Bug: curState is unexpectedly changed by another thread");
+      log.finest("the servlet output stream becomes not ready");


+      boolean successful =
+          writeState.compareAndSet(curState, curState.withReadyAndDrained(false));
+      LockSupport.unpark(parkingThread);
+      checkState(successful, "Bug: curState is unexpectedly changed by another thread");
+      log.finest("the servlet output stream becomes not ready");


+      boolean successful =
+          writeState.compareAndSet(curState, curState.withReadyAndDrained(false));
+      LockSupport.unpark(parkingThread);
+      checkState(successful, "Bug: curState is unexpectedly changed by another thread");
+      log.finest("the servlet output stream becomes not ready");


kannanjgithub · 2026-05-05T05:17:43Z

The Tomcat unit tests are timing out with this change.

Apply the readiness fix only to writeBytes actions, not flush. Flush can still go direct when isReady is true since it is less latency-sensitive. This prevents potential stalling of responses when a follow-up flush would be queued behind onWritePossible. Also update WriteState docs to reflect the new behavior. Fixes grpc#12723

mgustimz · 2026-05-05T14:49:04Z

Thanks for the review @copilot-pull-request-reviewer[bot]. Here is our response to your comments:

1. Stalling risk for flush() — Fixed
We narrowed the fix to apply only to writeBytes actions, not flush. Flush still follows the original behavior (only sets readyAndDrained=false when isReady() returns false). This prevents the stall scenario you described while still fixing the Tomcat write issue.

2. Missing regression test — Acknowledged
The existing AsyncServletOutputStreamWriterConcurrencyTest models the servlet container behavior differently from Tomcat. We agree a targeted test for back-to-back writes that stay ready would be valuable. We are considering how to add such a test without overcomplicating the test model.

3. Outdated documentation — Fixed
The WriteState class javadoc has been updated to reflect the new behavior:

For writeBytes actions, readyAndDrained always transitions to false after a direct write
For flush actions, readyAndDrained transitions to false only when isReady() returns false

We would appreciate any further feedback on the narrowed approach.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

        return;
      }
-      if (!isReady.getAsBoolean()) {
+      if (actionItem == writeAction) {


+      if (actionItem == writeAction) {
+        // For writeBytes, always set readyAndDrained to false even when isReady()
+        // returns true. Tomcat requires onWritePossible() to fire between writes,
+        // even if isReady() is still true. For flush, keep the original behavior
+        // since flush is less latency-sensitive and can safely wait for
+        // onWritePossible.


Use 'actionItem != flushAction' instead of 'actionItem == writeAction'. The latter was comparing incompatible types and would never evaluate to true. writeBytes passes writeAction.apply(...) (an ActionItem), not writeAction itself. Non-flush, non-complete actions are writeBytes. Fixes Copilot review comment on PR grpc#12790

mgustimz · 2026-05-06T07:34:27Z

Thanks for the updated review. Two issues raised:

1. Type comparison fix
You were right — actionItem == writeAction was comparing incompatible types (ActionItem vs BiFunction<byte[], Integer, ActionItem>). Since writeBytes() passes writeAction.apply(...) (the resulting ActionItem), not writeAction itself, this comparison could never be true. Fixed to actionItem != flushAction instead — any non-flush, non-complete action is a write action. Pushed at 44ee627.

2. Lincheck test model
You are correct that the existing AsyncServletOutputStreamWriterConcurrencyTest only triggers onWritePossible() after isReady() returns false, which no longer matches our intended contract. The test would need updating to model Tomcat behavior where onWritePossible() fires between consecutive writes even when isReady() stays true. We acknowledge this is a gap — we can address it in a follow-up if needed.

kannanjgithub · 2026-05-06T10:01:58Z

Unlike the complex Lincheck concurrency tests, we could just have a targeted JUnit test that uses a mock isReady supplier that always returns true.
AsyncServletOutputStreamWriterTest.java

Targeted JUnit test that uses a mock isReady supplier always returning true. This specifically tests the Tomcat scenario where isReady() stays true but writes can still fail because the previous write hasn't completed internally. The test verifies that multiple consecutive writes do not stall when isReady always returns true, exercising the new readyAndDrained=false path added for writeBytes actions. Addresses Copilot review comment on PR grpc#12790

kannanjgithub · 2026-05-06T11:01:08Z

+  public void writeBytes_alwaysReady_doesNotStall() throws IOException {
+    AtomicBoolean isReady = new AtomicBoolean(true);
+    List<byte[]> writtenData = new ArrayList<>();
+    AtomicInteger onWritePossibleCount = new AtomicInteger(0);


The compiler flags AtomicInteger because you are using it for a variable that is technically only being used by a single thread in this specific test scope. Error Prone's [UnnecessaryAsync]
check suggests using a non-concurrent type to reduce verbosity and overhead.

However, since you are inside a JUnit test and need to increment a counter inside a lambda (which requires variables to be "effectively final"), a simple int won't work.

The idiomatic way to satisfy both Error Prone and the "effectively final" constraint in gRPC tests is to use a single-element primitive array (e.g., int[]) instead of an AtomicInteger.

servlet: fix write when not ready in AsyncServletOutputStreamWriter

3f87103

Fixes grpc#12723

mgustimz force-pushed the fix/12723-servlet-clean branch from 7f3e6d6 to 3f87103 Compare May 2, 2026 16:40

kannanjgithub requested a review from Copilot May 5, 2026 05:05

Copilot started reviewing on behalf of kannanjgithub May 5, 2026 05:05 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

kannanjgithub requested a review from Copilot May 6, 2026 04:25

Copilot started reviewing on behalf of kannanjgithub May 6, 2026 04:25 View session

Copilot AI reviewed May 6, 2026

View reviewed changes

kannanjgithub reviewed May 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

servlet: fix write when not ready in AsyncServletOutputStreamWriter#12790

servlet: fix write when not ready in AsyncServletOutputStreamWriter#12790
mgustimz wants to merge 4 commits intogrpc:masterfrom
mgustimz:fix/12723-servlet-clean

mgustimz commented May 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

kannanjgithub commented May 5, 2026

Uh oh!

mgustimz commented May 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

mgustimz commented May 6, 2026

Uh oh!

kannanjgithub commented May 6, 2026

Uh oh!

kannanjgithub May 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mgustimz commented May 2, 2026

Description

Testing

Fixes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

kannanjgithub commented May 5, 2026

Uh oh!

mgustimz commented May 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mgustimz commented May 6, 2026

Uh oh!

kannanjgithub commented May 6, 2026

Uh oh!

kannanjgithub May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants