Skip to content

Commit 3e9c80e

Browse files
committed
Flush HTTP/2 request body per event-stream message
Outbound event streams previously lost their message boundaries twice on the write path: DefaultEventStreamWriter.toDataStream() wrapped the per-event EventPipeStream queue as a plain InputStream, and H2StreamRequestBody drained it with asInputStream().transferTo(out) — byte-oriented, so small events were coalesced into the 16 KB frame buffer and not sent until it filled or the stream closed. A request/response event protocol (send a small message, await the peer's reply, then send the next) would deadlock: the message never left the buffer, so the reply never came. Fix, via the existing DataStream.writeTo seam (no new public API, no reactive types in the blocking client): - EventPipeStream.writeMessagesTo(out) drains one queued event ByteBuffer at a time and flushes after each, preserving boundaries. - DefaultEventStreamWriter.toDataStream() returns a DataStream whose writeTo routes through writeMessagesTo (and still works as a normal InputStream for any other consumer). - H2StreamRequestBody drives the streaming write through body.writeTo(out) instead of asInputStream().transferTo(out). The default writeTo is still transferTo, so bulk uploads (S3 etc.) are unchanged. Tests: EventPipeStreamTest.writeMessagesToFlushesOncePerEvent (unit, one flush per event) and PerMessageFlushHttp2Test (integ, verified discriminator — a ping-pong body that awaits each message's echo before sending the next; passes with per-message flush, deadlocks to the @timeout on the coalescing path).
1 parent 7a16960 commit 3e9c80e

9 files changed

Lines changed: 403 additions & 34 deletions

File tree

core/src/main/java/software/amazon/smithy/java/core/serde/event/DefaultEventStreamWriter.java

Lines changed: 67 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,10 @@
55

66
package software.amazon.smithy.java.core.serde.event;
77

8+
import java.io.IOException;
9+
import java.io.InputStream;
10+
import java.io.OutputStream;
11+
import java.io.UncheckedIOException;
812
import java.util.Objects;
913
import java.util.concurrent.CountDownLatch;
1014
import java.util.concurrent.TimeUnit;
@@ -31,16 +35,19 @@ final class DefaultEventStreamWriter<IE extends SerializableStruct, T extends Se
3135
F extends Frame<?>>
3236
implements ProtocolEventStreamWriter<T, IE, F> {
3337
private static final InternalLogger LOGGER = InternalLogger.getLogger(DefaultEventStreamWriter.class);
38+
3439
/**
3540
* Default timeout to block waiting to write.
3641
*/
3742
private static final int WRITE_TIMEOUT_MILLIS = 10_000;
43+
3844
/**
3945
* This latch is used to ensure that the protocol handler writes the initial event
4046
* before any other event is written. Protocols that don't require the initial event still have
4147
* to unlatch the writer by bootstrapping it with a null value.
4248
*/
4349
private final CountDownLatch readyLatch = new CountDownLatch(1);
50+
4451
/**
4552
* Pipes bytes written by this writer to an input stream used
4653
* to send them over the wire.
@@ -184,8 +191,7 @@ private void checkState() {
184191

185192
@Override
186193
public EventStreamReader<T> asReader() {
187-
throw new UnsupportedOperationException(
188-
"This writer cannot be converted to a reader");
194+
throw new UnsupportedOperationException("This writer cannot be converted to a reader");
189195
}
190196

191197
@Override
@@ -233,6 +239,64 @@ public void close() {
233239
*/
234240
@Override
235241
public DataStream toDataStream() {
236-
return DataStream.ofInputStream(pipeStream);
242+
return new EventStreamDataStream(pipeStream);
243+
}
244+
245+
/**
246+
* A {@link DataStream} over the event pipe whose {@link #writeTo(OutputStream)} flushes after each event
247+
* (see {@link EventPipeStream#writeMessagesTo}), so a transport draining via {@code writeTo} emits one
248+
* frame per event. Still works as an ordinary unknown-length, non-replayable {@code InputStream}-backed
249+
* stream via {@link #asInputStream()}.
250+
*/
251+
private static final class EventStreamDataStream implements DataStream {
252+
private final EventPipeStream pipeStream;
253+
// The pipe is single-use: once drained (via writeTo/asInputStream) or closed it cannot be replayed.
254+
private final AtomicBoolean consumed = new AtomicBoolean(false);
255+
256+
EventStreamDataStream(EventPipeStream pipeStream) {
257+
this.pipeStream = pipeStream;
258+
}
259+
260+
@Override
261+
public void writeTo(OutputStream out) throws IOException {
262+
consumed.set(true);
263+
pipeStream.writeMessagesTo(out);
264+
}
265+
266+
@Override
267+
public InputStream asInputStream() {
268+
consumed.set(true);
269+
return pipeStream;
270+
}
271+
272+
@Override
273+
public long contentLength() {
274+
return -1;
275+
}
276+
277+
@Override
278+
public String contentType() {
279+
return null;
280+
}
281+
282+
@Override
283+
public boolean isReplayable() {
284+
return false;
285+
}
286+
287+
@Override
288+
public boolean isAvailable() {
289+
return !consumed.get();
290+
}
291+
292+
@Override
293+
public void close() {
294+
consumed.set(true);
295+
try {
296+
pipeStream.close();
297+
} catch (IOException e) {
298+
throw new UncheckedIOException("Failed to close event stream", e);
299+
}
300+
}
237301
}
238302
}

core/src/main/java/software/amazon/smithy/java/core/serde/event/EventPipeStream.java

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
import java.io.IOException;
99
import java.io.InputStream;
10+
import java.io.OutputStream;
1011
import java.nio.ByteBuffer;
1112
import java.util.Objects;
1213
import java.util.concurrent.ArrayBlockingQueue;
@@ -30,6 +31,7 @@
3031
*/
3132
final class EventPipeStream extends InputStream {
3233
private static final InternalLogger LOGGER = InternalLogger.getLogger(EventPipeStream.class);
34+
3335
/**
3436
* Poison pill used to signal the end of the stream.
3537
*/
@@ -160,6 +162,56 @@ public int read() throws IOException {
160162
return b & 0xFF;
161163
}
162164

165+
/**
166+
* Drains queued event messages to {@code out}, writing each whole and flushing after it. This keeps
167+
* per-event boundaries (unlike a byte-oriented {@code transferTo}), so a flushing transport sends each
168+
* event as its own frame instead of buffering. Blocks the consumer thread until {@link #complete()}
169+
* (returns) or {@link #completeWithError} (throws).
170+
*
171+
* @param out the sink to write messages to
172+
* @throws IOException if writing fails, the producer signalled an error, or the thread is interrupted
173+
*/
174+
void writeMessagesTo(OutputStream out) throws IOException {
175+
if (closed) {
176+
throw new IOException("Stream is closed");
177+
}
178+
179+
// Flush any buffer left partially consumed by a prior read() first.
180+
if (current != null && current != POISON_PILL) {
181+
writeBuffer(out, current);
182+
current = null;
183+
out.flush();
184+
}
185+
186+
while (true) {
187+
ByteBuffer message;
188+
try {
189+
message = queue.take();
190+
} catch (InterruptedException e) {
191+
Thread.currentThread().interrupt();
192+
throw new IOException("Interrupted while reading", e);
193+
}
194+
195+
if (message == POISON_PILL) {
196+
checkError();
197+
return;
198+
}
199+
200+
writeBuffer(out, message);
201+
out.flush();
202+
}
203+
}
204+
205+
private static void writeBuffer(OutputStream out, ByteBuffer message) throws IOException {
206+
if (message.hasArray()) {
207+
out.write(message.array(), message.arrayOffset() + message.position(), message.remaining());
208+
} else {
209+
byte[] tmp = new byte[message.remaining()];
210+
message.get(tmp);
211+
out.write(tmp);
212+
}
213+
}
214+
163215
@Override
164216
public int available() throws IOException {
165217
checkError();

core/src/test/java/software/amazon/smithy/java/core/serde/event/EventPipeStreamTest.java

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,12 @@
99

1010
import java.io.ByteArrayOutputStream;
1111
import java.io.IOException;
12+
import java.io.OutputStream;
1213
import java.nio.ByteBuffer;
1314
import java.nio.charset.StandardCharsets;
15+
import java.util.ArrayList;
16+
import java.util.List;
17+
import org.junit.jupiter.api.Test;
1418
import org.junit.jupiter.params.ParameterizedTest;
1519
import org.junit.jupiter.params.provider.MethodSource;
1620

@@ -83,4 +87,42 @@ static String[] sources() {
8387
static byte[] charToUtf8Bytes(char c) {
8488
return Character.toString(c).getBytes(StandardCharsets.UTF_8);
8589
}
90+
91+
@Test
92+
void writeMessagesToFlushesOncePerEvent() throws IOException {
93+
var pipe = new EventPipeStream();
94+
var events = new String[] {"syn", "event-two", "fin"};
95+
Thread.ofVirtual().start(() -> {
96+
for (var e : events) {
97+
pipe.write(ByteBuffer.wrap(e.getBytes(StandardCharsets.UTF_8)));
98+
}
99+
pipe.complete();
100+
});
101+
102+
// Records the bytes accumulated at each flush() so we can assert one flush == one whole event.
103+
var flushes = new ArrayList<String>();
104+
var current = new ByteArrayOutputStream();
105+
var recordingSink = new OutputStream() {
106+
@Override
107+
public void write(int b) {
108+
current.write(b);
109+
}
110+
111+
@Override
112+
public void write(byte[] b, int off, int len) {
113+
current.write(b, off, len);
114+
}
115+
116+
@Override
117+
public void flush() {
118+
flushes.add(current.toString(StandardCharsets.UTF_8));
119+
current.reset();
120+
}
121+
};
122+
123+
pipe.writeMessagesTo(recordingSink);
124+
125+
// One flush per event, in order, proving boundaries are preserved (transferTo would coalesce them).
126+
assertEquals(List.of("syn", "event-two", "fin"), flushes);
127+
}
86128
}

http/http-client/src/it/java/software/amazon/smithy/java/http/client/it/h2/BidirectionalStreamingHttp2Test.java

Lines changed: 11 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -26,24 +26,15 @@
2626
* Proves true HTTP/2 bidirectional (full-duplex) streaming: the response is read while the request body is
2727
* still open and unfinished.
2828
*
29-
* <p>The request body ({@link BlockingInputStream}) hands over a leading payload and then <em>blocks
30-
* indefinitely</em> before signalling end-of-stream — it only unblocks once the test thread releases it,
31-
* which the test does only <em>after</em> {@code client.send(...)} has returned and the leading echo has
32-
* been read. The server ({@link EchoHttp2ClientHandler}) sends response HEADERS as soon as the request
33-
* HEADERS arrive and echoes each request DATA frame straight back.
29+
* <p>The request body ({@link BlockingInputStream}) hands over a leading payload then blocks indefinitely,
30+
* unblocking only after the test releases it, which the test does only after {@code client.send(...)} has
31+
* returned and the leading echo has been read. The server ({@link EchoHttp2ClientHandler}) sends response
32+
* HEADERS on the request HEADERS and echoes each request DATA frame back.
3433
*
35-
* <p>This is a strict discriminator for duplex:
36-
* <ul>
37-
* <li><b>Full duplex (correct):</b> the client writes the request body on a background virtual thread
38-
* (see {@code DefaultHttpClient.sendForRoute}), so {@code send()} returns on the response HEADERS
39-
* while the body is still blocked. The test reads the leading echo, then releases the body to finish.</li>
40-
* <li><b>Serialized (broken):</b> if the client wrote the request body inline before reading the
41-
* response, {@code send()} would block inside the body write forever — the test thread that releases
42-
* the body never runs — and the test fails on the timeout.</li>
43-
* </ul>
44-
*
45-
* <p>Verified to be a real discriminator: forcing the client onto the inline-write path makes this test
46-
* hang to the timeout, while the duplex path passes.
34+
* <p>Verified discriminator: the client writes the request body on a background virtual thread (see
35+
* {@code DefaultHttpClient.sendForRoute}), so {@code send()} returns on the response HEADERS while the body
36+
* is still blocked. Forcing the inline-write path instead makes {@code send()} block in the body write
37+
* forever and the test times out.
4738
*/
4839
public class BidirectionalStreamingHttp2Test extends BaseHttpClientIntegTest {
4940

@@ -89,11 +80,9 @@ void readsResponseWhileRequestBodyIsStillOpen() throws Exception {
8980
assertEquals(200, response.statusCode());
9081

9182
try (InputStream in = response.body().asInputStream()) {
92-
// Read one full 16 KB frame of the echo while the request body is still blocked. Receiving
93-
// any response body before releasing the request is the proof that the two directions
94-
// interleave. (Reading the whole leading payload could block on bytes the client hasn't
95-
// flushed yet, since an OutputStream-backed body only flushes on a full frame buffer or
96-
// close — one frame is enough.)
83+
// Read one full 16 KB frame of the echo while the request body is still blocked. Getting any
84+
// response body before releasing the request proves the two directions interleave. (One
85+
// frame, not the whole payload, since the body only flushes on a full frame buffer.)
9786
int prefixLen = 16 * 1024;
9887
byte[] leadingEcho = readN(in, prefixLen);
9988
byte[] expectedPrefix = Arrays.copyOf(LEADING_PAYLOAD, prefixLen);

0 commit comments

Comments
 (0)