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.

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