Skip to content

Commit e898755

Browse files
committed
Address Copilot review items from #4251754607
- 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 #12790
1 parent c73e67a commit e898755

2 files changed

Lines changed: 9 additions & 9 deletions

File tree

servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -223,8 +223,9 @@ private void runOrBuffer(ActionItem actionItem) throws IOException {
223223
return;
224224
}
225225
if (actionItem == flushAction) {
226-
// flush path: only set readyAndDrained=false if isReady() returns false
227-
// If isReady() is still true, keep readyAndDrained=true so flush goes direct
226+
// flush path: only set readyAndDrained=false if isReady() returns false.
227+
// If isReady() is still true, keep readyAndDrained=true so subsequent
228+
// writes/flushes can go direct without unnecessary buffering.
228229
if (!isReady.getAsBoolean()) {
229230
boolean successful =
230231
writeState.compareAndSet(curState, curState.withReadyAndDrained(false));

servlet/src/test/java/io/grpc/servlet/AsyncServletOutputStreamWriterTest.java

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,10 @@
3232

3333
/**
3434
* Unit test for {@link AsyncServletOutputStreamWriter} with a mock isReady supplier.
35-
* Tests two scenarios: (1) writes with isReady always true, where onWritePossible()
36-
* is called between writes; (2) writes with readyAndDrained=false (triggered by first
37-
* write), where subsequent writes are buffered until onWritePossible() drains them.
35+
* Tests three scenarios:
36+
* (1) A write followed by another write without onWritePossible() is buffered.
37+
* (2) Consecutive writes with onWritePossible() between them succeed.
38+
* (3) When isReady() becomes false, writes are buffered until onWritePossible() drains them.
3839
*/
3940
@RunWith(JUnit4.class)
4041
public class AsyncServletOutputStreamWriterTest {
@@ -154,9 +155,8 @@ public void writeBytes_isReadyFalse_buffersUntilOnWritePossible() throws IOExcep
154155
byte[] data1 = new byte[]{1};
155156
writer.writeBytes(data1, 1);
156157

157-
// After first write, readyAndDrained=false, so any subsequent write is buffered
158-
isReady.set(false);
159-
158+
// After first write, readyAndDrained=false, so any subsequent write is buffered.
159+
// This buffering happens regardless of isReady() state (the Tomcat fix).
160160
// Second write - should be buffered since readyAndDrained=false
161161
byte[] data2 = new byte[]{2};
162162
writer.writeBytes(data2, 1);
@@ -165,7 +165,6 @@ public void writeBytes_isReadyFalse_buffersUntilOnWritePossible() throws IOExcep
165165
assertEquals("First write should complete, second buffered", 1, actions.size());
166166

167167
// Container calls onWritePossible to drain buffered writes
168-
isReady.set(true);
169168
writer.onWritePossible();
170169

171170
// Now both writes should have completed

0 commit comments

Comments
 (0)