-
Notifications
You must be signed in to change notification settings - Fork 26k
Add http request content stream support #111438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
2ff3afa
07f97cc
5d44698
839e575
83bec15
14a06ce
fbf5d28
116e802
d34d47f
90c8b5d
cd53cd7
7b66986
e0899b5
6dc8e46
f140b7d
660e4ca
aada8ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| package org.elasticsearch.http.netty4; | ||
|
|
||
| import org.apache.http.entity.ByteArrayEntity; | ||
| import org.apache.http.entity.ContentType; | ||
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
| import org.elasticsearch.ESNetty4IntegTestCase; | ||
| import org.elasticsearch.client.Request; | ||
| import org.elasticsearch.client.internal.node.NodeClient; | ||
| import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; | ||
| import org.elasticsearch.cluster.node.DiscoveryNodes; | ||
| import org.elasticsearch.common.bytes.BytesArray; | ||
| import org.elasticsearch.common.io.stream.NamedWriteableRegistry; | ||
| import org.elasticsearch.common.settings.ClusterSettings; | ||
| import org.elasticsearch.common.settings.IndexScopedSettings; | ||
| import org.elasticsearch.common.settings.Settings; | ||
| import org.elasticsearch.common.settings.SettingsFilter; | ||
| import org.elasticsearch.common.util.CollectionUtils; | ||
| import org.elasticsearch.features.NodeFeature; | ||
| import org.elasticsearch.http.HttpContent; | ||
| import org.elasticsearch.plugins.ActionPlugin; | ||
| import org.elasticsearch.plugins.Plugin; | ||
| import org.elasticsearch.rest.BaseRestHandler; | ||
| import org.elasticsearch.rest.RestController; | ||
| import org.elasticsearch.rest.RestHandler; | ||
| import org.elasticsearch.rest.RestRequest; | ||
| import org.elasticsearch.rest.RestResponse; | ||
| import org.elasticsearch.rest.RestStatus; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.Collection; | ||
| import java.util.List; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
| import java.util.function.Consumer; | ||
| import java.util.function.Predicate; | ||
| import java.util.function.Supplier; | ||
|
|
||
| public class Netty4HttpRequestStreamIT extends ESNetty4IntegTestCase { | ||
|
|
||
| @Override | ||
| protected Collection<Class<? extends Plugin>> nodePlugins() { | ||
| return CollectionUtils.concatLists(List.of(RequestContentStreamPlugin.class), super.nodePlugins()); | ||
| } | ||
|
|
||
| @Override | ||
| protected boolean addMockHttpTransport() { | ||
| return false; // enable http | ||
| } | ||
|
|
||
| public void testBasicStream() throws IOException { | ||
| var totalBytes = 1024 * 1024; | ||
| var request = new Request("POST", RequestContentStreamPlugin.ROUTE); | ||
| request.setEntity(new ByteArrayEntity(randomByteArrayOfLength(totalBytes), ContentType.APPLICATION_JSON)); | ||
|
|
||
| var respose = getRestClient().performRequest(request); | ||
| assertEquals(200, respose.getStatusLine().getStatusCode()); | ||
| var gotTotalBytes = new BytesArray(respose.getEntity().getContent().readAllBytes()).utf8ToString(); | ||
| assertEquals("" + totalBytes, gotTotalBytes); | ||
| } | ||
|
|
||
| public static class RequestContentStreamPlugin extends Plugin implements ActionPlugin { | ||
|
|
||
| static final String ROUTE = "/_test/request-stream/basic"; | ||
| private static final Logger LOGGER = LogManager.getLogger("StreamRestHandler"); | ||
|
|
||
| @Override | ||
| public Collection<RestHandler> getRestHandlers( | ||
| Settings settings, | ||
| NamedWriteableRegistry namedWriteableRegistry, | ||
| RestController restController, | ||
| ClusterSettings clusterSettings, | ||
| IndexScopedSettings indexScopedSettings, | ||
| SettingsFilter settingsFilter, | ||
| IndexNameExpressionResolver indexNameExpressionResolver, | ||
| Supplier<DiscoveryNodes> nodesInCluster, | ||
| Predicate<NodeFeature> clusterSupportsFeature | ||
| ) { | ||
| return List.of(new BaseRestHandler() { | ||
| @Override | ||
| public String getName() { | ||
| return ROUTE; | ||
| } | ||
|
|
||
| @Override | ||
| public List<Route> routes() { | ||
| return List.of(new Route(RestRequest.Method.POST, ROUTE)); | ||
| } | ||
|
|
||
| @Override | ||
| protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) { | ||
| return channel -> { | ||
|
|
||
| // Example of handling streamed content | ||
|
|
||
| var contentStream = request.contentStream(); | ||
| var totalReceivedBytes = new AtomicInteger(); | ||
|
|
||
| Consumer<HttpContent.Chunk> contentConsumer = (chunk) -> { | ||
| LOGGER.info("got next chunk with size {}", chunk.bytes().length()); | ||
| totalReceivedBytes.addAndGet(chunk.bytes().length()); | ||
| if (chunk.isLast() == false) { | ||
| contentStream.request(1024 * 10); // ask for 10kb, will round up to 16kb | ||
|
mhl-b marked this conversation as resolved.
Outdated
|
||
| } else { | ||
| channel.sendResponse(new RestResponse(RestStatus.OK, Integer.toString(totalReceivedBytes.get()))); | ||
| } | ||
| }; | ||
|
|
||
| contentStream.setHandler(contentConsumer); // must setup handler before first request | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder, rather than another layer of indirection here could we add a chunk-consuming method to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added public interface RequestBodyChunkConsumer extends RestChannelConsumer {
void handleChunk(RestChannel channel, ReleasableBytesReference chunk, boolean isLast);
}and wiring for handler into body stream in if (request.isStreamedContent()) {
assert action instanceof RequestBodyChunkConsumer;
var chunkConsumer = (RequestBodyChunkConsumer) action;
request.contentStream().setHandler((chunk, isLast) -> chunkConsumer.handleChunk(channel, chunk, isLast));
}implementation looks like this @Override
protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) {
return new RequestBodyChunkConsumer() {
int totalBytes = 0;
@Override
public void handleChunk(RestChannel channel, ReleasableBytesReference chunk, boolean isLast) {
try (chunk) {
totalBytes += chunk.length();
if (isLast == false) {
request.contentStream().requestBytes(1024);
} else {
channel.sendResponse(new RestResponse(RestStatus.OK, Integer.toString(totalBytes)));
}
}
}
@Override
public void accept(RestChannel channel) throws Exception {
request.contentStream().requestBytes(1024); // ask for first chunk
}
};
} |
||
| contentStream.request(1024); // initiate first chunk, will round up to 8kb | ||
| }; | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| package org.elasticsearch.http.netty4; | ||
|
|
||
| import io.netty.handler.codec.http.HttpObjectAggregator; | ||
| import io.netty.handler.codec.http.HttpRequest; | ||
|
|
||
| import org.elasticsearch.http.HttpPreRequest; | ||
| import org.elasticsearch.http.netty4.internal.HttpHeadersAuthenticatorUtils; | ||
|
|
||
| import java.util.function.Predicate; | ||
|
|
||
| public class Netty4HttpAggregator extends HttpObjectAggregator { | ||
|
mhl-b marked this conversation as resolved.
|
||
| private static final Predicate<HttpPreRequest> IGNORE_TEST = (req) -> req.uri().startsWith("/_test/request-stream") == false; | ||
|
|
||
| private final Predicate<HttpPreRequest> decider; | ||
| private boolean shouldAggregate; | ||
|
|
||
| public Netty4HttpAggregator(int maxContentLength) { | ||
| this(maxContentLength, IGNORE_TEST); | ||
| } | ||
|
|
||
| public Netty4HttpAggregator(int maxContentLength, Predicate<HttpPreRequest> decider) { | ||
| super(maxContentLength); | ||
| this.decider = decider; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean acceptInboundMessage(Object msg) throws Exception { | ||
| if (msg instanceof HttpRequest request) { | ||
| var preReq = HttpHeadersAuthenticatorUtils.asHttpPreRequest(request); | ||
| shouldAggregate = decider.test(preReq); | ||
| } | ||
| if (shouldAggregate) { | ||
| return super.acceptInboundMessage(msg); | ||
| } else { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,18 +9,24 @@ | |
| package org.elasticsearch.http.netty4; | ||
|
|
||
| import io.netty.buffer.ByteBuf; | ||
| import io.netty.buffer.Unpooled; | ||
| import io.netty.channel.Channel; | ||
| import io.netty.channel.ChannelDuplexHandler; | ||
| import io.netty.channel.ChannelFuture; | ||
| import io.netty.channel.ChannelHandlerContext; | ||
| import io.netty.channel.ChannelPromise; | ||
| import io.netty.handler.codec.compression.JdkZlibEncoder; | ||
| import io.netty.handler.codec.http.DefaultFullHttpRequest; | ||
| import io.netty.handler.codec.http.DefaultHttpContent; | ||
| import io.netty.handler.codec.http.DefaultHttpResponse; | ||
| import io.netty.handler.codec.http.DefaultLastHttpContent; | ||
| import io.netty.handler.codec.http.EmptyHttpHeaders; | ||
| import io.netty.handler.codec.http.FullHttpRequest; | ||
| import io.netty.handler.codec.http.HttpContent; | ||
| import io.netty.handler.codec.http.HttpObject; | ||
| import io.netty.handler.codec.http.HttpRequest; | ||
| import io.netty.handler.codec.http.HttpResponse; | ||
| import io.netty.handler.codec.http.LastHttpContent; | ||
| import io.netty.handler.ssl.SslCloseCompletionEvent; | ||
| import io.netty.util.ReferenceCountUtil; | ||
| import io.netty.util.concurrent.Future; | ||
|
|
@@ -70,6 +76,12 @@ private record ChunkedWrite(PromiseCombiner combiner, ChannelPromise onDone, Chu | |
| @Nullable | ||
| private ChunkedWrite currentChunkedWrite; | ||
|
|
||
| /** | ||
| * HTTP request content stream for current request, it's null if there is no current request or request is fully-aggregated | ||
| */ | ||
| @Nullable | ||
| private Netty4HttpRequestContentStream currentRequestStream; | ||
|
|
||
| /* | ||
| * The current read and write sequence numbers. Read sequence numbers are attached to requests in the order they are read from the | ||
| * channel, and then transferred to responses. A response is not written to the channel context until its sequence number matches the | ||
|
|
@@ -109,23 +121,48 @@ public Netty4HttpPipeliningHandler( | |
| public void channelRead(final ChannelHandlerContext ctx, final Object msg) { | ||
| activityTracker.startActivity(); | ||
| try { | ||
| assert msg instanceof FullHttpRequest : "Should have fully aggregated message already but saw [" + msg + "]"; | ||
| final FullHttpRequest fullHttpRequest = (FullHttpRequest) msg; | ||
| final Netty4HttpRequest netty4HttpRequest; | ||
| if (fullHttpRequest.decoderResult().isFailure()) { | ||
| final Throwable cause = fullHttpRequest.decoderResult().cause(); | ||
| final Exception nonError; | ||
| if (cause instanceof Error) { | ||
| ExceptionsHelper.maybeDieOnAnotherThread(cause); | ||
| nonError = new Exception(cause); | ||
| if (msg instanceof HttpRequest request) { | ||
| if (request.decoderResult().isFailure()) { | ||
| final Throwable cause = request.decoderResult().cause(); | ||
| final Exception nonError; | ||
| if (cause instanceof Error) { | ||
| ExceptionsHelper.maybeDieOnAnotherThread(cause); | ||
| nonError = new Exception(cause); | ||
| } else { | ||
| nonError = (Exception) cause; | ||
| } | ||
| netty4HttpRequest = new Netty4HttpRequest(readSequence++, (FullHttpRequest) request, nonError); | ||
| } else { | ||
| nonError = (Exception) cause; | ||
| if (request instanceof FullHttpRequest fullHttpRequest) { | ||
| currentRequestStream = null; | ||
|
mhl-b marked this conversation as resolved.
Outdated
|
||
| netty4HttpRequest = new Netty4HttpRequest(readSequence++, fullHttpRequest); | ||
| } else { | ||
| var contentStream = new Netty4HttpRequestContentStream(ctx.channel()); | ||
| currentRequestStream = contentStream; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, can we assert
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can be non-null if previous request was stream too. When we see HttpRequest that means we received all parts of previous request - all HttpContent's and single LastHttpContent. At this point previous parts should be either in previous stream queue or processed by rest handler.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We set
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, sorry I lost track of my own changes :) In normal circumstances should be null. If request is not properly terminated (no last content) and there is new request then
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ++ to a test, but could we also |
||
| netty4HttpRequest = new Netty4HttpRequest( | ||
| readSequence++, | ||
| new DefaultFullHttpRequest( | ||
| request.protocolVersion(), | ||
| request.method(), | ||
| request.uri(), | ||
| Unpooled.EMPTY_BUFFER, | ||
| request.headers(), | ||
| EmptyHttpHeaders.INSTANCE | ||
| ), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we just using a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, it's one of the non-essential small things that touches many lines of code and bloat PR. Still need to address this, no reason to pass FullHttpRequest here. |
||
| contentStream | ||
| ); | ||
| } | ||
| } | ||
| netty4HttpRequest = new Netty4HttpRequest(readSequence++, fullHttpRequest, nonError); | ||
| handlePipelinedRequest(ctx, netty4HttpRequest); | ||
| } else { | ||
| netty4HttpRequest = new Netty4HttpRequest(readSequence++, fullHttpRequest); | ||
| assert msg instanceof HttpContent; | ||
| assert currentRequestStream != null; | ||
| currentRequestStream.handleNettyContent((HttpContent) msg); | ||
| if (msg instanceof LastHttpContent) { | ||
| currentRequestStream = null; | ||
| } | ||
| } | ||
| handlePipelinedRequest(ctx, netty4HttpRequest); | ||
| } finally { | ||
| activityTracker.stopActivity(); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.