Skip to content

Commit 3f81019

Browse files
committed
Split flush and writeBytes into separate branches
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 #12790
1 parent 1ef9eb8 commit 3f81019

2 files changed

Lines changed: 29 additions & 21 deletions

File tree

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

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -222,24 +222,24 @@ private void runOrBuffer(ActionItem actionItem) throws IOException {
222222
if (actionItem == completeAction) {
223223
return;
224224
}
225-
if (actionItem != flushAction) {
226-
// This is a writeBytes action. Always set readyAndDrained to false even when
227-
// isReady() returns true. Tomcat requires onWritePossible() to fire between
228-
// writes, even if isReady() is still true.
229-
boolean successful =
230-
writeState.compareAndSet(curState, curState.withReadyAndDrained(false));
231-
LockSupport.unpark(parkingThread);
232-
checkState(successful, "Bug: curState is unexpectedly changed by another thread");
233-
log.finest("direct write: cleared readyAndDrained, next writes buffered");
234-
// For flush, only set to false if isReady() returns false.
235-
// If isReady() is still true, keep readyAndDrained true so flush goes direct.
225+
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
236228
if (!isReady.getAsBoolean()) {
237229
boolean successful =
238230
writeState.compareAndSet(curState, curState.withReadyAndDrained(false));
239231
LockSupport.unpark(parkingThread);
240232
checkState(successful, "Bug: curState is unexpectedly changed by another thread");
241233
log.finest("the servlet output stream becomes not ready");
242234
}
235+
} else {
236+
// writeBytes path: always set readyAndDrained=false even when isReady()
237+
// returns true. Tomcat requires onWritePossible() to fire between writes.
238+
boolean successful =
239+
writeState.compareAndSet(curState, curState.withReadyAndDrained(false));
240+
LockSupport.unpark(parkingThread);
241+
checkState(successful, "Bug: curState is unexpectedly changed by another thread");
242+
log.finest("direct write: cleared readyAndDrained, next writes buffered");
243243
}
244244
} else { // buffer to the writeChain
245245
writeChain.offer(actionItem);

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

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -82,11 +82,12 @@ public void writeBytes_alwaysReady_doesNotStall() throws IOException {
8282
}
8383

8484
/**
85-
* Test that writeBytes with isReady transitioning from true to false eventually
86-
* drains via onWritePossible.
85+
* Test that when isReady() returns false, writes are buffered and only drain
86+
* when onWritePossible() is called. This models the container behavior where
87+
* writes block until the container signals the stream is ready again.
8788
*/
8889
@Test
89-
public void writeBytes_isReadyBecomesFalse_triggersOnWritePossible() throws IOException {
90+
public void writeBytes_isReadyFalse_buffersUntilOnWritePossible() throws IOException {
9091
AtomicBoolean isReady = new AtomicBoolean(true);
9192
List<String> actions = new ArrayList<>();
9293

@@ -106,22 +107,29 @@ public void writeBytes_isReadyBecomesFalse_triggersOnWritePossible() throws IOEx
106107
AsyncServletOutputStreamWriter writer =
107108
new AsyncServletOutputStreamWriter(writeAction, flushAction, completeAction, isReadySupplier, new Log() {});
108109

109-
// Initial onWritePossible
110+
// Initial onWritePossible to set readyAndDrained=true
110111
writer.onWritePossible();
111112

112113
// First write - isReady is true, goes direct, readyAndDrained=false
113114
byte[] data1 = new byte[]{1};
114115
writer.writeBytes(data1, 1);
115116

116-
// Container calls onWritePossible after first write completes internally
117-
// isReady is still true but readyAndDrained is false, so buffered writes drain
118-
writer.onWritePossible();
117+
// Simulate isReady becoming false (container buffer full)
118+
isReady.set(false);
119119

120-
// Second write after isReady returns to true
121-
isReady.set(true);
120+
// Second write - should be buffered since isReady=false
122121
byte[] data2 = new byte[]{2};
123122
writer.writeBytes(data2, 1);
124123

125-
assertEquals("Two writes should complete", 2, actions.size());
124+
// Only the first write should have executed
125+
assertEquals("First write should complete, second buffered", 1, actions.size());
126+
127+
// Container calls onWritePossible when ready to accept more data
128+
// isReady is still false, but onWritePossible will drain anyway
129+
isReady.set(true);
130+
writer.onWritePossible();
131+
132+
// Now both writes should have completed
133+
assertEquals("Both writes should complete after onWritePossible", 2, actions.size());
126134
}
127135
}

0 commit comments

Comments
 (0)