From 8509f44a45013ebdc1232e1e997b5125c3781e66 Mon Sep 17 00:00:00 2001 From: Nick Tindall Date: Tue, 25 Jun 2024 16:54:13 +1000 Subject: [PATCH 1/5] Add leak tracking and re-enable chunked continuation test --- .../http/netty4/Netty4ChunkedContinuationsIT.java | 4 +++- muted-tests.yml | 3 --- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/elasticsearch/http/netty4/Netty4ChunkedContinuationsIT.java b/modules/transport-netty4/src/internalClusterTest/java/org/elasticsearch/http/netty4/Netty4ChunkedContinuationsIT.java index 5ad1152d65e85..cfb69c7ec41fe 100644 --- a/modules/transport-netty4/src/internalClusterTest/java/org/elasticsearch/http/netty4/Netty4ChunkedContinuationsIT.java +++ b/modules/transport-netty4/src/internalClusterTest/java/org/elasticsearch/http/netty4/Netty4ChunkedContinuationsIT.java @@ -74,6 +74,7 @@ import org.elasticsearch.test.MockLog; import org.elasticsearch.test.junit.annotations.TestLogging; import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.transport.LeakTracker; import org.elasticsearch.transport.TransportService; import org.elasticsearch.transport.netty4.Netty4Utils; import org.elasticsearch.xcontent.ToXContentObject; @@ -316,13 +317,14 @@ public void onFailure(Exception exception) { private static Releasable withResourceTracker() { assertNull(refs); final var latch = new CountDownLatch(1); - refs = AbstractRefCounted.of(latch::countDown); + refs = LeakTracker.wrap(AbstractRefCounted.of(latch::countDown)); return () -> { refs.decRef(); try { safeAwait(latch); } finally { refs = null; + System.gc(); } }; } diff --git a/muted-tests.yml b/muted-tests.yml index f8e69c90acc70..b9d7006b62019 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -85,9 +85,6 @@ tests: - class: org.elasticsearch.xpack.transform.transforms.TransformIndexerTests method: testMaxPageSearchSizeIsResetToConfiguredValue issue: https://github.com/elastic/elasticsearch/issues/109844 -- class: "org.elasticsearch.http.netty4.Netty4ChunkedContinuationsIT" - issue: "https://github.com/elastic/elasticsearch/issues/109866" - method: "testClientCancellation" - class: "org.elasticsearch.test.jvm_crash.JvmCrashIT" issue: "https://github.com/elastic/elasticsearch/issues/110052" method: "testJvmCrash" From b0a9cb603d36d3fdcffd7b25ad95536fe3291d6b Mon Sep 17 00:00:00 2001 From: Nick Tindall Date: Tue, 25 Jun 2024 17:35:19 +1000 Subject: [PATCH 2/5] Fix suspected issue --- .../http/netty4/Netty4HttpPipeliningHandler.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java index c9beeef246703..7add86ed9218b 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java @@ -283,13 +283,13 @@ private void finishChunkedWrite() { public void onResponse(ChunkedRestResponseBodyPart continuation) { // always fork a fresh task to avoid stack overflow assert Transports.assertDefaultThreadContext(threadContext); - channel.eventLoop() - .execute( - () -> channel.writeAndFlush( - new Netty4ChunkedHttpContinuation(writeSequence, continuation, finishingWrite.combiner()), - finishingWrite.onDone() // pass the terminal listener/promise along the line - ) + channel.eventLoop().execute(() -> { + channel.writeAndFlush( + new Netty4ChunkedHttpContinuation(writeSequence, continuation, finishingWrite.combiner()), + finishingWrite.onDone() // pass the terminal listener/promise along the line ); + checkShutdown(); + }); checkShutdown(); } From 92388e2e2ae3b8a805a42948f804a79bc6d121cf Mon Sep 17 00:00:00 2001 From: Nick Tindall Date: Thu, 27 Jun 2024 11:27:15 +1000 Subject: [PATCH 3/5] Revert "Fix suspected issue" This reverts commit b0a9cb603d36d3fdcffd7b25ad95536fe3291d6b. --- .../http/netty4/Netty4HttpPipeliningHandler.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java index 7add86ed9218b..c9beeef246703 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java @@ -283,13 +283,13 @@ private void finishChunkedWrite() { public void onResponse(ChunkedRestResponseBodyPart continuation) { // always fork a fresh task to avoid stack overflow assert Transports.assertDefaultThreadContext(threadContext); - channel.eventLoop().execute(() -> { - channel.writeAndFlush( - new Netty4ChunkedHttpContinuation(writeSequence, continuation, finishingWrite.combiner()), - finishingWrite.onDone() // pass the terminal listener/promise along the line + channel.eventLoop() + .execute( + () -> channel.writeAndFlush( + new Netty4ChunkedHttpContinuation(writeSequence, continuation, finishingWrite.combiner()), + finishingWrite.onDone() // pass the terminal listener/promise along the line + ) ); - checkShutdown(); - }); checkShutdown(); } From 30c20016a51e741b603a363cc83f48de69b47e11 Mon Sep 17 00:00:00 2001 From: Nick Tindall Date: Thu, 27 Jun 2024 11:27:53 +1000 Subject: [PATCH 4/5] Fix Netty4ChunkedContinuationsIT#testClientCancellation --- .../elasticsearch/http/netty4/Netty4ChunkedContinuationsIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/elasticsearch/http/netty4/Netty4ChunkedContinuationsIT.java b/modules/transport-netty4/src/internalClusterTest/java/org/elasticsearch/http/netty4/Netty4ChunkedContinuationsIT.java index 21437c06c803d..d2f7f6ab61977 100644 --- a/modules/transport-netty4/src/internalClusterTest/java/org/elasticsearch/http/netty4/Netty4ChunkedContinuationsIT.java +++ b/modules/transport-netty4/src/internalClusterTest/java/org/elasticsearch/http/netty4/Netty4ChunkedContinuationsIT.java @@ -643,10 +643,10 @@ public void close() { @Override public void accept(RestChannel channel) { - localRefs.mustIncRef(); client.execute(TYPE, new Request(), new RestActionListener<>(channel) { @Override protected void processResponse(Response response) { + localRefs.mustIncRef(); channel.sendResponse(RestResponse.chunked(RestStatus.OK, response.getResponseBodyPart(), () -> { // cancellation notification only happens while processing a continuation, not while computing // the next one; prompt cancellation requires use of something like RestCancellableNodeClient From 8561e5397961c2e3c9ae2161ff951c6441620bbc Mon Sep 17 00:00:00 2001 From: Nick Tindall Date: Thu, 27 Jun 2024 11:30:08 +1000 Subject: [PATCH 5/5] Unmute test again --- muted-tests.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index c8624a1b60011..748f6f463f345 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -73,9 +73,6 @@ tests: - class: org.elasticsearch.xpack.transform.transforms.TransformIndexerTests method: testMaxPageSearchSizeIsResetToConfiguredValue issue: https://github.com/elastic/elasticsearch/issues/109844 -- class: org.elasticsearch.http.netty4.Netty4ChunkedContinuationsIT - method: testClientCancellation - issue: https://github.com/elastic/elasticsearch/issues/109866 - class: org.elasticsearch.index.store.FsDirectoryFactoryTests method: testStoreDirectory issue: https://github.com/elastic/elasticsearch/issues/110210