Skip to content

Commit 85d9990

Browse files
authored
Replace auto-read with proper flow-control in HTTP pipeline (#127817)
Re-applying #126441 (cf. #127259) with: - the extra `FlowControlHandler` needed to ensure one-chunk-per-read semantics (also present in #127259). - no extra `read()` after exhausting a `Netty4HttpRequestBodyStream` (the bug behind #127391 and #127391). See #127111 for related tests.
1 parent 077b6b9 commit 85d9990

File tree

15 files changed

+427
-1081
lines changed

15 files changed

+427
-1081
lines changed

docs/changelog/127817.yaml

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 127817
2+
summary: Replace auto-read with proper flow-control in HTTP pipeline
3+
area: Network
4+
type: enhancement
5+
issues: []

modules/transport-netty4/src/internalClusterTest/java/org/elasticsearch/http/netty4/Netty4IncrementalRequestHandlingIT.java

+1-7
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,6 @@ public void testClientConnectionCloseMidStream() throws Exception {
208208

209209
// await stream handler is ready and request full content
210210
var handler = clientContext.awaitRestChannelAccepted(opaqueId);
211-
assertBusy(() -> assertNotEquals(0, handler.stream.bufSize()));
212211

213212
assertFalse(handler.isClosed());
214213

@@ -218,7 +217,6 @@ public void testClientConnectionCloseMidStream() throws Exception {
218217
assertEquals(requestTransmittedLength, handler.readUntilClose());
219218

220219
assertTrue(handler.isClosed());
221-
assertEquals(0, handler.stream.bufSize());
222220
}
223221
}
224222

@@ -235,7 +233,6 @@ public void testServerCloseConnectionMidStream() throws Exception {
235233

236234
// await stream handler is ready and request full content
237235
var handler = clientContext.awaitRestChannelAccepted(opaqueId);
238-
assertBusy(() -> assertNotEquals(0, handler.stream.bufSize()));
239236
assertFalse(handler.isClosed());
240237

241238
// terminate connection on server and wait resources are released
@@ -244,7 +241,6 @@ public void testServerCloseConnectionMidStream() throws Exception {
244241
handler.channel.request().getHttpChannel().close();
245242
assertThat(safeGet(exceptionFuture), instanceOf(ClosedChannelException.class));
246243
assertTrue(handler.isClosed());
247-
assertBusy(() -> assertEquals(0, handler.stream.bufSize()));
248244
}
249245
}
250246

@@ -260,7 +256,6 @@ public void testServerExceptionMidStream() throws Exception {
260256

261257
// await stream handler is ready and request full content
262258
var handler = clientContext.awaitRestChannelAccepted(opaqueId);
263-
assertBusy(() -> assertNotEquals(0, handler.stream.bufSize()));
264259
assertFalse(handler.isClosed());
265260

266261
// terminate connection on server and wait resources are released
@@ -272,7 +267,6 @@ public void testServerExceptionMidStream() throws Exception {
272267
final var exception = asInstanceOf(RuntimeException.class, safeGet(exceptionFuture));
273268
assertEquals(ServerRequestHandler.SIMULATED_EXCEPTION_MESSAGE, exception.getMessage());
274269
safeAwait(handler.closedLatch);
275-
assertBusy(() -> assertEquals(0, handler.stream.bufSize()));
276270
}
277271
}
278272

@@ -313,7 +307,7 @@ public void testClientBackpressure() throws Exception {
313307
});
314308
handler.readBytes(partSize);
315309
}
316-
assertTrue(handler.stream.hasLast());
310+
assertTrue(handler.receivedLastChunk);
317311
}
318312
}
319313

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the "Elastic License
4+
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
5+
* Public License v 1"; you may not use this file except in compliance with, at
6+
* your election, the "Elastic License 2.0", the "GNU Affero General Public
7+
* License v3.0 only", or the "Server Side Public License, v 1".
8+
*/
9+
10+
package org.elasticsearch.http.netty4;
11+
12+
import io.netty.channel.ChannelDuplexHandler;
13+
import io.netty.channel.ChannelHandlerContext;
14+
import io.netty.util.concurrent.ScheduledFuture;
15+
16+
import org.apache.logging.log4j.LogManager;
17+
import org.apache.logging.log4j.Logger;
18+
import org.elasticsearch.common.time.TimeProvider;
19+
import org.elasticsearch.common.util.concurrent.FutureUtils;
20+
21+
import java.util.concurrent.TimeUnit;
22+
23+
/**
24+
* When channel auto-read is disabled handlers are responsible to read from channel.
25+
* But it's hard to detect when read is missing. This helper class print warnings
26+
* when no reads where detected in given time interval. Normally, in tests, 10 seconds is enough
27+
* to avoid test hang for too long, but can be increased if needed.
28+
*/
29+
class MissingReadDetector extends ChannelDuplexHandler {
30+
31+
private static final Logger logger = LogManager.getLogger(MissingReadDetector.class);
32+
33+
private final long interval;
34+
private final TimeProvider timer;
35+
private boolean pendingRead;
36+
private long lastRead;
37+
private ScheduledFuture<?> checker;
38+
39+
MissingReadDetector(TimeProvider timer, long missingReadIntervalMillis) {
40+
this.interval = missingReadIntervalMillis;
41+
this.timer = timer;
42+
}
43+
44+
@Override
45+
public void handlerAdded(ChannelHandlerContext ctx) throws Exception {
46+
checker = ctx.channel().eventLoop().scheduleAtFixedRate(() -> {
47+
if (pendingRead == false) {
48+
long now = timer.absoluteTimeInMillis();
49+
if (now >= lastRead + interval) {
50+
logger.warn("chan-id={} haven't read from channel for [{}ms]", ctx.channel().id(), (now - lastRead));
51+
}
52+
}
53+
}, interval, interval, TimeUnit.MILLISECONDS);
54+
super.handlerAdded(ctx);
55+
}
56+
57+
@Override
58+
public void handlerRemoved(ChannelHandlerContext ctx) throws Exception {
59+
if (checker != null) {
60+
FutureUtils.cancel(checker);
61+
}
62+
super.handlerRemoved(ctx);
63+
}
64+
65+
@Override
66+
public void read(ChannelHandlerContext ctx) throws Exception {
67+
pendingRead = true;
68+
ctx.read();
69+
}
70+
71+
@Override
72+
public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {
73+
assert ctx.channel().config().isAutoRead() == false : "auto-read must be always disabled";
74+
pendingRead = false;
75+
lastRead = timer.absoluteTimeInMillis();
76+
ctx.fireChannelRead(msg);
77+
}
78+
}

modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpAggregator.java

+4
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import io.netty.handler.codec.http.HttpObjectAggregator;
1616
import io.netty.handler.codec.http.HttpRequest;
1717
import io.netty.handler.codec.http.HttpRequestDecoder;
18+
import io.netty.handler.codec.http.LastHttpContent;
1819

1920
import org.elasticsearch.http.HttpPreRequest;
2021
import org.elasticsearch.http.netty4.internal.HttpHeadersAuthenticatorUtils;
@@ -48,6 +49,9 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
4849
}
4950
if (aggregating || msg instanceof FullHttpRequest) {
5051
super.channelRead(ctx, msg);
52+
if (msg instanceof LastHttpContent == false) {
53+
ctx.read(); // HttpObjectAggregator is tricky with auto-read off, it might not call read again, calling on its behalf
54+
}
5155
} else {
5256
streamContentSizeHandler.channelRead(ctx, msg);
5357
}

modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpContentSizeHandler.java

+4
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,7 @@ private void handleRequest(ChannelHandlerContext ctx, HttpRequest request) {
123123
isContinueExpected = true;
124124
} else {
125125
ctx.writeAndFlush(EXPECTATION_FAILED_CLOSE.retainedDuplicate()).addListener(ChannelFutureListener.CLOSE);
126+
ctx.read();
126127
return;
127128
}
128129
}
@@ -136,6 +137,7 @@ private void handleRequest(ChannelHandlerContext ctx, HttpRequest request) {
136137
decoder.reset();
137138
}
138139
ctx.writeAndFlush(TOO_LARGE.retainedDuplicate()).addListener(ChannelFutureListener.CLOSE_ON_FAILURE);
140+
ctx.read();
139141
} else {
140142
ignoreContent = false;
141143
currentContentLength = 0;
@@ -150,11 +152,13 @@ private void handleRequest(ChannelHandlerContext ctx, HttpRequest request) {
150152
private void handleContent(ChannelHandlerContext ctx, HttpContent msg) {
151153
if (ignoreContent) {
152154
msg.release();
155+
ctx.read();
153156
} else {
154157
currentContentLength += msg.content().readableBytes();
155158
if (currentContentLength > maxContentLength) {
156159
msg.release();
157160
ctx.writeAndFlush(TOO_LARGE_CLOSE.retainedDuplicate()).addListener(ChannelFutureListener.CLOSE);
161+
ctx.read();
158162
} else {
159163
ctx.fireChannelRead(msg);
160164
}

0 commit comments

Comments
 (0)