backport http content copy removal #117303 (#117599)

Author: mhl-b

build-tools-internal/src/main/resources/forbidden/es-server-signatures.txt

@@ -171,5 +171,3 @@ org.elasticsearch.cluster.SnapshotDeletionsInProgress$Entry#<init>(java.lang.Str
 @defaultMessage Use a Thread constructor with a name, anonymous threads are more difficult to debug
 java.lang.Thread#<init>(java.lang.Runnable)
 java.lang.Thread#<init>(java.lang.ThreadGroup, java.lang.Runnable)
-
-org.elasticsearch.common.bytes.BytesReference#copyBytes(org.elasticsearch.common.bytes.BytesReference) @ This method is a subject for removal. Copying bytes is prone to performance regressions and unnecessary allocations.

docs/changelog/117303.yaml

@@ -0,0 +1,5 @@
+pr: 117303
+summary: Remove HTTP content copies
+area: Network
+type: enhancement
+issues: []

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

@@ -89,7 +89,7 @@ public List<Route> routes() {
 
                 @Override
                 protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException {
-                    var content = request.releasableContent();
+                    var content = request.content();
                     var iter = content.iterator();
                     return (chan) -> {
                         request.getHttpRequest().release();

qa/system-indices/src/main/java/org/elasticsearch/system/indices/SystemIndicesQA.java

@@ -10,6 +10,7 @@
 
 package org.elasticsearch.system.indices;
 
+import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.admin.indices.create.CreateIndexRequest;
 import org.elasticsearch.action.index.IndexRequest;
 import org.elasticsearch.client.internal.node.NodeClient;
@@ -179,12 +180,12 @@ public List<Route> routes() {
 
         @Override
         protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException {
+            var content = request.requiredContent();
             IndexRequest indexRequest = new IndexRequest(".net-new-system-index-primary");
-            indexRequest.source(request.requiredContent(), request.getXContentType());
+            indexRequest.source(content, request.getXContentType());
             indexRequest.id(request.param("id"));
             indexRequest.setRefreshPolicy(request.param("refresh"));
-
-            return channel -> client.index(indexRequest, new RestToXContentListener<>(channel));
+            return channel -> client.index(indexRequest, ActionListener.withRef(new RestToXContentListener<>(channel), content));
         }
 
         @Override

server/src/main/java/org/elasticsearch/action/ActionListener.java

@@ -475,4 +475,12 @@ static <T, R extends AutoCloseable> void runWithResource(
         ActionListener.run(ActionListener.runBefore(listener, resource::close), l -> action.accept(l, resource));
     }
 
+    /**
+     * Increments ref count and returns a listener that will decrement ref count on listener completion.
+     */
+    static <Response> ActionListener<Response> withRef(ActionListener<Response> listener, RefCounted ref) {
+        ref.mustIncRef();
+        return releaseAfter(listener, ref::decRef);
+    }
+
 }

server/src/main/java/org/elasticsearch/common/bytes/BytesReference.java

@@ -74,29 +74,6 @@ static ByteBuffer[] toByteBuffers(BytesReference reference) {
         }
     }
 
-    /**
-     * Allocates new buffer and copy bytes from given BytesReference.
-     *
-     * @deprecated copying bytes is a right place for performance regression and unnecessary allocations.
-     * This method exists to serve very few places that struggle to handle reference counted buffers.
-     */
-    @Deprecated(forRemoval = true)
-    static BytesReference copyBytes(BytesReference bytesReference) {
-        byte[] arr = new byte[bytesReference.length()];
-        int offset = 0;
-        final BytesRefIterator iterator = bytesReference.iterator();
-        try {
-            BytesRef slice;
-            while ((slice = iterator.next()) != null) {
-                System.arraycopy(slice.bytes, slice.offset, arr, offset, slice.length);
-                offset += slice.length;
-            }
-            return new BytesArray(arr);
-        } catch (IOException e) {
-            throw new AssertionError(e);
-        }
-    }
-
     /**
      * Returns BytesReference composed of the provided ByteBuffers.
      */

server/src/main/java/org/elasticsearch/http/HttpTracer.java

@@ -94,7 +94,7 @@ HttpTracer maybeLogRequest(RestRequest restRequest, @Nullable Exception e) {
 
     private void logFullContent(RestRequest restRequest) {
         try (var stream = HttpBodyTracer.getBodyOutputStream(restRequest.getRequestId(), HttpBodyTracer.Type.REQUEST)) {
-            restRequest.releasableContent().writeTo(stream);
+            restRequest.content().writeTo(stream);
         } catch (Exception e2) {
             assert false : e2; // no real IO here
         }

server/src/main/java/org/elasticsearch/rest/RestRequest.java

@@ -23,7 +23,6 @@
 import org.elasticsearch.core.CheckedConsumer;
 import org.elasticsearch.core.Nullable;
 import org.elasticsearch.core.RestApiVersion;
-import org.elasticsearch.core.SuppressForbidden;
 import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.core.Tuple;
 import org.elasticsearch.http.HttpBody;
@@ -303,22 +302,13 @@ public boolean isFullContent() {
         return httpRequest.body().isFull();
     }
 
-    /**
-     * Returns a copy of HTTP content. The copy is GC-managed and does not require reference counting.
-     * Please use {@link #releasableContent()} to avoid content copy.
-     */
-    @SuppressForbidden(reason = "temporarily support content copy while migrating RestHandlers to ref counted pooled buffers")
-    public BytesReference content() {
-        return BytesReference.copyBytes(releasableContent());
-    }
-
     /**
      * Returns a direct reference to the network buffer containing the request body. The HTTP layers will release their references to this
      * buffer as soon as they have finished the synchronous steps of processing the request on the network thread, which will by default
      * release the buffer back to the pool where it may be re-used for another request. If you need to keep the buffer alive past the end of
      * these synchronous steps, acquire your own reference to this buffer and release it once it's no longer needed.
      */
-    public ReleasableBytesReference releasableContent() {
+    public ReleasableBytesReference content() {
         this.contentConsumed = true;
         var bytes = httpRequest.body().asFull().bytes();
         if (bytes.hasReferences() == false) {
@@ -338,32 +328,19 @@ public HttpBody.Stream contentStream() {
         return httpRequest.body().asStream();
     }
 
-    private void ensureContent() {
+    /**
+     * Returns reference to the network buffer of HTTP content or throw an exception if the body or content type is missing.
+     * See {@link #content()}.
+     */
+    public ReleasableBytesReference requiredContent() {
         if (hasContent() == false) {
             throw new ElasticsearchParseException("request body is required");
         } else if (xContentType.get() == null) {
             throwValidationException("unknown content type");
         }
-    }
-
-    /**
-     * @return copy of the request body or throw an exception if the body or content type is missing.
-     * See {@link #content()}. Please use {@link #requiredReleasableContent()} to avoid content copy.
-     */
-    public final BytesReference requiredContent() {
-        ensureContent();
         return content();
     }
 
-    /**
-     * Returns reference to the network buffer of HTTP content or throw an exception if the body or content type is missing.
-     * See {@link #releasableContent()}. It's a recommended method to handle HTTP content without copying it.
-     */
-    public ReleasableBytesReference requiredReleasableContent() {
-        ensureContent();
-        return releasableContent();
-    }
-
     private static void throwValidationException(String msg) {
         ValidationException unknownContentType = new ValidationException();
         unknownContentType.addValidationError(msg);
@@ -596,7 +573,7 @@ public final boolean hasContentOrSourceParam() {
      * if you need to handle the absence request content gracefully.
      */
     public final XContentParser contentOrSourceParamParser() throws IOException {
-        Tuple<XContentType, BytesReference> tuple = contentOrSourceParam();
+        Tuple<XContentType, ReleasableBytesReference> tuple = contentOrSourceParam();
         return XContentHelper.createParserNotCompressed(parserConfig, tuple.v2(), tuple.v1().xContent().type());
     }
 
@@ -607,7 +584,7 @@ public final XContentParser contentOrSourceParamParser() throws IOException {
      */
     public final void withContentOrSourceParamParserOrNull(CheckedConsumer<XContentParser, IOException> withParser) throws IOException {
         if (hasContentOrSourceParam()) {
-            Tuple<XContentType, BytesReference> tuple = contentOrSourceParam();
+            Tuple<XContentType, ReleasableBytesReference> tuple = contentOrSourceParam();
             try (XContentParser parser = XContentHelper.createParserNotCompressed(parserConfig, tuple.v2(), tuple.v1())) {
                 withParser.accept(parser);
             }
@@ -620,7 +597,7 @@ public final void withContentOrSourceParamParserOrNull(CheckedConsumer<XContentP
      * Get the content of the request or the contents of the {@code source} param or throw an exception if both are missing.
      * Prefer {@link #contentOrSourceParamParser()} or {@link #withContentOrSourceParamParserOrNull(CheckedConsumer)} if you need a parser.
      */
-    public final Tuple<XContentType, BytesReference> contentOrSourceParam() {
+    public final Tuple<XContentType, ReleasableBytesReference> contentOrSourceParam() {
         if (hasContentOrSourceParam() == false) {
             throw new ElasticsearchParseException("request body or source parameter is required");
         } else if (hasContent()) {
@@ -636,7 +613,7 @@ public final Tuple<XContentType, BytesReference> contentOrSourceParam() {
         if (xContentType == null) {
             throwValidationException("Unknown value for source_content_type [" + typeParam + "]");
         }
-        return new Tuple<>(xContentType, bytes);
+        return new Tuple<>(xContentType, ReleasableBytesReference.wrap(bytes));
     }
 
     public ParsedMediaType getParsedAccept() {

server/src/main/java/org/elasticsearch/rest/RestRequestFilter.java

@@ -45,10 +45,10 @@ public boolean hasContent() {
                 }
 
                 @Override
-                public ReleasableBytesReference releasableContent() {
+                public ReleasableBytesReference content() {
                     if (filteredBytes == null) {
                         Tuple<XContentType, Map<String, Object>> result = XContentHelper.convertToMap(
-                            restRequest.requiredReleasableContent(),
+                            restRequest.requiredContent(),
                             true,
                             restRequest.getXContentType()
                         );

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestPutStoredScriptAction.java

@@ -8,6 +8,7 @@
  */
 package org.elasticsearch.rest.action.admin.cluster;
 
+import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.admin.cluster.storedscripts.PutStoredScriptRequest;
 import org.elasticsearch.action.admin.cluster.storedscripts.TransportPutStoredScriptAction;
 import org.elasticsearch.client.internal.node.NodeClient;
@@ -57,6 +58,10 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client
             request.getXContentType(),
             StoredScriptSource.parse(content, xContentType)
         );
-        return channel -> client.execute(TransportPutStoredScriptAction.TYPE, putRequest, new RestToXContentListener<>(channel));
+        return channel -> client.execute(
+            TransportPutStoredScriptAction.TYPE,
+            putRequest,
+            ActionListener.withRef(new RestToXContentListener<>(channel), content)
+        );
     }
 }