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

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

Description

In runOrBuffer(), after a direct write of writeBytes data, always set readyAndDrained to false even when isReady() returns true. This ensures subsequent writes go through the container thread via onWritePossible(), matching Tomcat's requirement that onWritePossible() fire between consecutive writes.

For flush actions, the original behavior is preserved: readyAndDrained only becomes false if isReady() returns false after the flush. This prevents response stalls for flush-heavy workloads.

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

New unit test AsyncServletOutputStreamWriterTest with mock isReady supplier
Existing lincheck concurrency test (models different container behavior)
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.

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.

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

Error Prone flags AtomicInteger when only used by a single thread. The onWritePossibleCount variable was not needed for the test's verification — writtenData.size() is sufficient to confirm writes completed. Addressed kannanjgithub's review comment on PR grpc#12790

Copilot

Pull request overview

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

- Fix test: add onWritePossible() calls between writes to model container callback behavior. Without this, writes are buffered and never drained, so writtenData.size() never reaches 5. - Remove unused assertTrue import (was causing compilation error) - Update log message to reflect actual state transition: 'direct write: cleared readyAndDrained, next writes buffered' (only for writeBytes path; flush path keeps original message) Addressed Copilot review comments on PR grpc#12790

Copilot

Pull request overview

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

Fix two issues flagged by Copilot: 1. flush path was nested inside writeBytes branch and never executed for flush 2. Second CAS could fail when isReady()=false (same curState used twice) New structure: - flushAction: only set readyAndDrained=false if isReady()=false (original behavior) - writeBytes: always set readyAndDrained=false (Tomcat fix) Also renamed second test to match actual behavior: writeBytes_isReadyBecomesFalse_isBuffered -> writeBytes_isReadyFalse_buffersUntilOnWritePossible Fixed test to actually set isReady=false and verify second write is buffered. Addressed Copilot comments on PR grpc#12790

Copilot

Pull request overview

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

- Rename first test to writeBytes_onWritePossibleBetweenWrites_succeeds to accurately describe what's being tested - Remove misleading 'subsequent writes buffered' comment since onWritePossible resets readyAndDrained=true on each iteration - Fix comment in second test: isReady was set to true before onWritePossible, not 'still false' - Simplify class-level Javadoc to describe the two scenarios tested - Add clarifications to first test Javadoc about isReady staying true Addressed Copilot comments at commit 3f81019 on PR grpc#12790

Copilot

Pull request overview

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

Add a new test 'writeBytes_withoutOnWritePossible_isBuffered' that: - Calls writeBytes, checks it executes - Calls writeBytes AGAIN without onWritePossible in between - Asserts second write is NOT executed (buffered) - Then calls onWritePossible and asserts buffered write drains This validates the core Tomcat fix: writeBytes sets readyAndDrained=false, so subsequent writes must go through onWritePossible callback. Also update existing tests' comments to clarify buffering is triggered by readyAndDrained=false (from first write), not by isReady being false. Addressed Copilot review on PR grpc#12790

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

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

+ * Tests two scenarios: (1) writes with isReady always true, where onWritePossible()
+ * is called between writes; (2) writes with readyAndDrained=false (triggered by first
+ * write), where subsequent writes are buffered until onWritePossible() drains them.


+  /**
+   * Test that when isReady() returns false after a direct write, subsequent writes
+   * are buffered and only drain when onWritePossible() is called.
+   */
+  @Test
+  public void writeBytes_isReadyFalse_buffersUntilOnWritePossible() throws IOException {
+    AtomicBoolean isReady = new AtomicBoolean(true);
+    List<String> actions = new ArrayList<>();
+
+    BiFunction<byte[], Integer, ActionItem> writeAction =
+        (bytes, numBytes) -> () -> {
+          actions.add("write");
+        };
+
+    ActionItem flushAction = () -> {
+      actions.add("flush");
+    };
+
+    ActionItem completeAction = () -> {};
+
+    BooleanSupplier isReadySupplier = () -> isReady.get();
+
+    AsyncServletOutputStreamWriter writer =
+        new AsyncServletOutputStreamWriter(writeAction, flushAction, completeAction, isReadySupplier, new Log() {});
+
+    // Initial onWritePossible to set readyAndDrained=true
+    writer.onWritePossible();
+
+    // First write - goes direct, readyAndDrained becomes false
+    byte[] data1 = new byte[]{1};
+    writer.writeBytes(data1, 1);
+
+    // After first write, readyAndDrained=false, so any subsequent write is buffered
+    isReady.set(false);
+
+    // Second write - should be buffered since readyAndDrained=false
+    byte[] data2 = new byte[]{2};
+    writer.writeBytes(data2, 1);
+
+    // Only the first write should have executed
+    assertEquals("First write should complete, second buffered", 1, actions.size());
+
+    // Container calls onWritePossible to drain buffered writes
+    isReady.set(true);
+    writer.onWritePossible();
+
+    // Now both writes should have completed
+    assertEquals("Both writes should complete after onWritePossible", 2, actions.size());
+  }


-      if (!isReady.getAsBoolean()) {
+      if (actionItem == flushAction) {
+        // flush path: only set readyAndDrained=false if isReady() returns false
+        // If isReady() is still true, keep readyAndDrained=true so flush goes direct


- Update class Javadoc to reflect 3 tests (was 2) - Third test: remove misleading isReady toggling since buffering is triggered by readyAndDrained=false (set after first write), not by isReady() state. Keep isReady always true to match the actual scenario being tested. - Fix comment: 'buffering happens regardless of isReady state' - Fix inline comment in runOrBuffer flush path: 'subsequent writes/flushes can go direct without unnecessary buffering' Reviewed-by: copilot-pull-request-reviewer[bot] Fixes grpc#12790

Old test was misleading - it toggled isReady but buffering was triggered by readyAndDrained=false (from first write), not isReady state. New test 'flush_isReadyTrue_goesDirect_isReadyFalse_buffers' tests the flush path in runOrBuffer(): - Flush with isReady=true: goes direct (readyAndDrained stays true) - Flush with isReady=false: buffers (readyAndDrained becomes false) This directly validates the flush behavior preservation mentioned in the PR. Reviewed-by: copilot-pull-request-reviewer[bot] Fixes grpc#12790

Copilot

Pull request overview

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

+      if (actionItem == flushAction) {
+        // flush path: only set readyAndDrained=false if isReady() returns false.
+        // If isReady() is still true, keep readyAndDrained=true so subsequent
+        // writes/flushes can go direct without unnecessary buffering.
+        if (!isReady.getAsBoolean()) {
+          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");
+        }
+      } else {
+        // writeBytes path: always set readyAndDrained=false even when isReady()
+        // returns true. Tomcat requires onWritePossible() to fire between writes.
        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");
+        log.finest("direct write: cleared readyAndDrained, next writes buffered");
      }


+    // Flush with isReady=true - should go direct (readyAndDrained stays true)
+    writer.flush();
+    assertEquals("Flush should execute directly when isReady=true", 1, actions.size());
+
+    // Simulate isReady becoming false (container buffer full)
+    isReady.set(false);
+
+    // Next flush - should be buffered since isReady=false
+    writer.flush();
+
+    // Only the first flush should have executed
+    assertEquals("Second flush should be buffered when isReady=false", 1, actions.size());
+


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

Comment thread servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java Outdated

Comment thread servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java Outdated

Comment thread servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java Outdated

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

Comment thread servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java Outdated

Comment thread servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java Outdated

kannanjgithub reviewed May 6, 2026

View reviewed changes

Comment thread servlet/src/test/java/io/grpc/servlet/AsyncServletOutputStreamWriterTest.java Outdated

kannanjgithub requested a review from Copilot May 7, 2026 11:00

Copilot started reviewing on behalf of kannanjgithub May 7, 2026 11:00 View session