Skip to content

Commit dd4a496

Browse files
danielkecdanielkec
authored
[4.x] Remove duplicate Http/2 frame split in client #10815 (#10829)
* Remove duplicate Http/2 frame split in client #10815 Signed-off-by: Daniel Kec <[email protected]>
1 parent 4c806fd commit dd4a496

File tree

4 files changed

+338
-18
lines changed

4 files changed

+338
-18
lines changed

‎webclient/http2/src/main/java/io/helidon/webclient/http2/Http2ClientStream.java‎

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@
4343
import io.helidon.http.http2.Http2Ping;
4444
import io.helidon.http.http2.Http2Priority;
4545
import io.helidon.http.http2.Http2RstStream;
46-
import io.helidon.http.http2.Http2Setting;
4746
import io.helidon.http.http2.Http2Settings;
4847
import io.helidon.http.http2.Http2Stream;
4948
import io.helidon.http.http2.Http2StreamState;
@@ -340,7 +339,7 @@ public void writeData(BufferData entityBytes, boolean endOfStream) {
340339
: 0),
341340
streamId);
342341
Http2FrameData frameData = new Http2FrameData(frameHeader, entityBytes);
343-
splitAndWrite(frameData);
342+
write(frameData, frameData.header().flags(Http2FrameTypes.DATA).endOfStream());
344343
}
345344

346345
/**
@@ -488,16 +487,6 @@ private Http2Headers readHeaders(Http2HuffmanDecoder decoder, boolean mergeWithP
488487
return http2Headers;
489488
}
490489

491-
private void splitAndWrite(Http2FrameData frameData) {
492-
int maxFrameSize = this.serverSettings.value(Http2Setting.MAX_FRAME_SIZE).intValue();
493-
494-
// Split to frames if bigger than max frame size
495-
Http2FrameData[] frames = frameData.split(maxFrameSize);
496-
for (Http2FrameData frame : frames) {
497-
write(frame, frame.header().flags(Http2FrameTypes.DATA).endOfStream());
498-
}
499-
}
500-
501490
private void write(Http2FrameData frameData, boolean endOfStream) {
502491
this.state = Http2StreamState.checkAndGetState(this.state,
503492
frameData.header().type(),

‎webclient/tests/http2/src/test/java/io/helidon/webclient/tests/http2/MaxFrameSizeTest.java‎

Lines changed: 245 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,245 @@
1+
/*
2+
* Copyright (c) 2025 Oracle and/or its affiliates.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package io.helidon.webclient.tests.http2;
18+
19+
import java.nio.charset.StandardCharsets;
20+
import java.time.Duration;
21+
import java.util.List;
22+
import java.util.concurrent.CompletableFuture;
23+
import java.util.concurrent.CopyOnWriteArrayList;
24+
import java.util.concurrent.ExecutionException;
25+
import java.util.concurrent.TimeUnit;
26+
import java.util.concurrent.TimeoutException;
27+
28+
import io.helidon.http.HeaderValues;
29+
import io.helidon.http.Method;
30+
import io.helidon.http.Status;
31+
import io.helidon.logging.common.LogConfig;
32+
import io.helidon.webclient.http2.Http2Client;
33+
import io.helidon.webclient.http2.Http2ClientResponse;
34+
35+
import io.netty.buffer.Unpooled;
36+
import io.netty.handler.codec.http.HttpResponseStatus;
37+
import io.netty.handler.codec.http2.DefaultHttp2Headers;
38+
import io.netty.handler.codec.http2.Http2Headers;
39+
import io.netty.handler.codec.http2.Http2Settings;
40+
import org.junit.jupiter.api.AfterEach;
41+
import org.junit.jupiter.api.BeforeEach;
42+
import org.junit.jupiter.api.Test;
43+
44+
import static org.hamcrest.MatcherAssert.assertThat;
45+
import static org.hamcrest.Matchers.is;
46+
47+
class MaxFrameSizeTest {
48+
49+
private static final Duration TIMEOUT = Duration.ofSeconds(10);
50+
private static MockHttp2Server mockHttp2Server;
51+
private static int serverPort;
52+
private static CompletableFuture<Void> SETTINGS_SENT;
53+
private static CompletableFuture<Void> SETTINGS_ACKED;
54+
private static CompletableFuture<Void> SEND_HEADERS;
55+
56+
@BeforeEach
57+
void beforeAll() {
58+
LogConfig.configureRuntime();
59+
SETTINGS_SENT = new CompletableFuture<>();
60+
SETTINGS_ACKED = CompletableFuture.completedFuture(null);
61+
SEND_HEADERS = CompletableFuture.completedFuture(null);
62+
63+
mockHttp2Server = MockHttp2Server.builder()
64+
.onHeaders((ctx, streamId, headers, payload, encoder) -> {
65+
var mockServerCtx = mockHttp2Server.mockServerContext().stream(streamId);
66+
mockServerCtx.setHeaders(headers);
67+
68+
switch (headers.path().toString()) {
69+
case "/trigger-settings" -> {
70+
Http2Headers h = new DefaultHttp2Headers()
71+
.status(HttpResponseStatus.OK.codeAsText());
72+
73+
encoder.writeHeaders(ctx, streamId, h, 0, false, ctx.newPromise());
74+
75+
int testMaxFrameSize = Integer.parseInt(headers.get("test-max-frame-size").toString());
76+
encoder.writeData(ctx,
77+
streamId,
78+
Unpooled.wrappedBuffer(("Settings size change triggered: " + testMaxFrameSize).getBytes()),
79+
0,
80+
true,
81+
ctx.newPromise());
82+
83+
encoder.writeSettings(ctx,
84+
Http2Settings.defaultSettings().maxFrameSize(testMaxFrameSize),
85+
ctx.newPromise());
86+
}
87+
case "/test-batch" -> {
88+
}
89+
case "/test-batch-interim-settings" -> {
90+
var maxFrameSizeHeader = headers.get("test-max-frame-size");
91+
int testMaxFrameSize = Integer.parseInt(maxFrameSizeHeader.toString());
92+
encoder.writeSettings(ctx,
93+
Http2Settings.defaultSettings().maxFrameSize(testMaxFrameSize),
94+
ctx.newPromise());
95+
SETTINGS_SENT.complete(null);
96+
}
97+
default -> {
98+
Http2Headers h = new DefaultHttp2Headers()
99+
.status(HttpResponseStatus.NOT_FOUND.codeAsText());
100+
encoder.writeHeaders(ctx, streamId, h, 0, true, ctx.newPromise());
101+
}
102+
}
103+
104+
})
105+
.onSettingsAck((ctx, encoder) -> {
106+
SETTINGS_ACKED.complete(null);
107+
})
108+
.onData((ctx, streamId, data, padding, endOfStream, encoder) -> {
109+
var mckCtx = mockHttp2Server.mockServerContext().stream(streamId);
110+
List<Integer> frameSizeList = (List<Integer>) mckCtx
111+
.ctx()
112+
.computeIfAbsent("frame-size", s -> new CopyOnWriteArrayList<Integer>());
113+
114+
frameSizeList.add(data.readableBytes());
115+
116+
if (endOfStream) {
117+
Http2Headers h = new DefaultHttp2Headers()
118+
.status(HttpResponseStatus.OK.codeAsText());
119+
encoder.writeHeaders(ctx, streamId, h, 0, false, ctx.newPromise());
120+
var msg = frameSizeList.stream()
121+
.filter(i -> i > 0)
122+
.map(String::valueOf)
123+
.toList()
124+
.toString();
125+
encoder.writeData(ctx,
126+
streamId,
127+
Unpooled.wrappedBuffer((msg).getBytes()),
128+
0,
129+
true,
130+
ctx.newPromise());
131+
132+
}
133+
return 0;
134+
})
135+
.build();
136+
137+
serverPort = mockHttp2Server.port();
138+
}
139+
140+
@AfterEach
141+
void afterEach() {
142+
mockHttp2Server.shutdown();
143+
}
144+
145+
@Test
146+
void maxFrameChangeMidConnection() throws InterruptedException {
147+
Http2Client
148+
client = Http2Client.builder()
149+
.shareConnectionCache(false)
150+
.connectTimeout(Duration.ofMinutes(10))
151+
.baseUri("http://localhost:" + serverPort)
152+
.build();
153+
154+
// Check default MAX_FRAME_SIZE=16384 is set
155+
try (Http2ClientResponse res = client
156+
.method(Method.GET)
157+
.path("/test-batch")
158+
.header(HeaderValues.create("test-batch-size", String.valueOf(30_000)))
159+
.priorKnowledge(true)
160+
.submit("A".repeat(17_000))) {
161+
162+
assertThat(res.status(), is(Status.OK_200));
163+
assertThat(res.as(String.class), is("[16384, 616]"));
164+
}
165+
166+
// Trigger server to change MAX_FRAME_SIZE=18_000
167+
try (Http2ClientResponse res = client
168+
.method(Method.GET)
169+
.path("/trigger-settings")
170+
.header(HeaderValues.create("test-max-frame-size", String.valueOf(18_000)))
171+
.priorKnowledge(true)
172+
.request()) {
173+
174+
assertThat(res.status(), is(Status.OK_200));
175+
}
176+
177+
// Test that the client honors MAX_FRAME_SIZE=18_000
178+
try (Http2ClientResponse res = client
179+
.method(Method.GET)
180+
.path("/test-batch")
181+
.header(HeaderValues.create("test-batch-size", String.valueOf(30_000)))
182+
.priorKnowledge(true)
183+
.submit("A".repeat(20_000))) {
184+
185+
assertThat(res.status(), is(Status.OK_200));
186+
assertThat(res.as(String.class), is("[18000, 2000]"));
187+
}
188+
}
189+
190+
@Test
191+
void maxFrameChangeMidStream() throws InterruptedException {
192+
Http2Client
193+
client = Http2Client.builder()
194+
.shareConnectionCache(false)
195+
.connectTimeout(Duration.ofMinutes(10))
196+
.baseUri("http://localhost:" + serverPort)
197+
.build();
198+
199+
//Check default MAX_FRAME_SIZE=16384 is set
200+
try (Http2ClientResponse res = client
201+
.method(Method.GET)
202+
.path("/test-batch")
203+
.header(HeaderValues.create("test-batch-size", String.valueOf(30_000)))
204+
.priorKnowledge(true)
205+
.submit("A".repeat(20_000))) {
206+
207+
assertThat(res.status(), is(Status.OK_200));
208+
assertThat(res.as(String.class), is("[16384, 3616]"));
209+
}
210+
211+
SEND_HEADERS = new CompletableFuture<>();
212+
SETTINGS_ACKED = new CompletableFuture<>();
213+
214+
// Test that the client honors MAX_FRAME_SIZE=18_000
215+
try (Http2ClientResponse res = client
216+
.method(Method.GET)
217+
.path("/test-batch-interim-settings")
218+
.header(HeaderValues.create("test-max-frame-size", String.valueOf(18_000)))
219+
.header(HeaderValues.create("test-batch-size", String.valueOf(30_000)))
220+
.priorKnowledge(true)
221+
.outputStream(out -> {
222+
var future = new CompletableFuture<Void>();
223+
SETTINGS_ACKED = future;
224+
// The first data frame just forces headers to be sent
225+
out.write("A".repeat(10).getBytes(StandardCharsets.UTF_8));
226+
out.flush();
227+
// Now wait till the new setting are sent from the server
228+
try {
229+
SETTINGS_SENT.get(TIMEOUT.getSeconds(), TimeUnit.SECONDS);
230+
SETTINGS_ACKED.get(TIMEOUT.getSeconds(), TimeUnit.SECONDS);
231+
} catch (InterruptedException | ExecutionException | TimeoutException e) {
232+
throw new RuntimeException(e);
233+
}
234+
// Write more than MAX_FRAME_SIZE=18_000 to observe correct split
235+
out.write("A".repeat(19_000).getBytes(StandardCharsets.UTF_8));
236+
out.flush();
237+
SEND_HEADERS.complete(null);
238+
out.close();
239+
})) {
240+
241+
assertThat(res.status(), is(Status.OK_200));
242+
assertThat(res.as(String.class), is("[10, 18000, 1000]"));
243+
}
244+
}
245+
}

0 commit comments

Comments
 (0)