TESTS: Fix Buf Leaks in HttpReadWriteHandlerTests by original-brownbear · Pull Request #32377 · elastic/elasticsearch

original-brownbear · 2018-07-25T19:06:16Z

Release all ref counted things that weren't getting properly released
Mannually force channel promise to be completed because mock channel doesn't do it and it prevents one release call in io.netty.channel.ChannelOutboundHandlerAdapter#write from firing
Now passes a few thousand iterations with leak detection set to paranoid

* Release all ref counted things that weren't getting properly released * Mannually force channel promise to be completed because mock channel doesn't do it and it prevents one `release` call in `io.netty.channel.ChannelOutboundHandlerAdapter#write` from firing * Now passes a few thousand iterations with leak detection set to paranoid

elasticmachine · 2018-07-25T19:06:19Z

Pinging @elastic/es-core-infra

Tim-Brooks

A few comments.

Tim-Brooks · 2018-07-25T19:20:29Z

plugins/transport-nio/src/test/java/org/elasticsearch/http/nio/HttpReadWriteHandlerTests.java

-        List<FlushOperation> flushOperations = handler.writeToBytes(handler.createWriteOperation(context, response, (v, e) -> {}));
-
+        List<FlushOperation> flushOperations = handler.writeToBytes(handler.createWriteOperation(context, response, (v, e) -> {
+            ReferenceCountUtil.release(v);


This does not do anything. The listener takes Void. v will always be null.

oops my bad for keeping this around =>reverting :)

Tim-Brooks · 2018-07-25T19:20:56Z

plugins/transport-nio/src/test/java/org/elasticsearch/http/nio/HttpReadWriteHandlerTests.java

+        }));
+        handler.close();
        FlushOperation flushOperation = flushOperations.get(0);
+        ((ChannelPromise) flushOperation.getListener()).setSuccess();


No need for this as the listener does not release anything.

@tbrooks8 see my PR description it does trigger (that calls msg.release) the callback in io.netty.channel.ChannelOutboundHandlerAdapter#write

If I out comment it I run into:

Created at: io.netty.buffer.PooledByteBufAllocator.newHeapBuffer(PooledByteBufAllocator.java:314) io.netty.buffer.AbstractByteBufAllocator.heapBuffer(AbstractByteBufAllocator.java:162) io.netty.buffer.AbstractByteBufAllocator.heapBuffer(AbstractByteBufAllocator.java:153) io.netty.buffer.AbstractByteBufAllocator.buffer(AbstractByteBufAllocator.java:111) io.netty.handler.codec.http.HttpObjectEncoder.encode(HttpObjectEncoder.java:92) io.netty.handler.codec.MessageToMessageEncoder.write(MessageToMessageEncoder.java:88) io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:738) io.netty.channel.AbstractChannelHandlerContext.invokeWrite(AbstractChannelHandlerContext.java:730) io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:816) io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:723) io.netty.handler.codec.MessageToMessageEncoder.write(MessageToMessageEncoder.java:111) io.netty.handler.codec.MessageToMessageCodec.write(MessageToMessageCodec.java:116) io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:738) io.netty.channel.AbstractChannelHandlerContext.invokeWrite(AbstractChannelHandlerContext.java:730) io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:816) io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:723) org.elasticsearch.http.nio.cors.NioCorsHandler.write(NioCorsHandler.java:86) io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:738) io.netty.channel.AbstractChannelHandlerContext.invokeWrite(AbstractChannelHandlerContext.java:730) io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:816) io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:723) org.elasticsearch.http.nio.NioHttpPipeliningHandler.write(NioHttpPipeliningHandler.java:71) io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:738) io.netty.channel.AbstractChannelHandlerContext.invokeWriteAndFlush(AbstractChannelHandlerContext.java:801) io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:814) io.netty.channel.AbstractChannelHandlerContext.writeAndFlush(AbstractChannelHandlerContext.java:794) io.netty.channel.DefaultChannelPipeline.writeAndFlush(DefaultChannelPipeline.java:1036) io.netty.channel.AbstractChannel.writeAndFlush(AbstractChannel.java:305) org.elasticsearch.http.nio.NettyAdaptor.write(NettyAdaptor.java:113) org.elasticsearch.http.nio.HttpReadWriteHandler.writeToBytes(HttpReadWriteHandler.java:99) org.elasticsearch.http.nio.HttpReadWriteHandlerTests.executeCorsRequest(HttpReadWriteHandlerTests.java:356) org.elasticsearch.http.nio.HttpReadWriteHandlerTests.testThatStringLiteralWorksOnMatch(HttpReadWriteHandlerTests.java:310) jdk.internal.reflect.GeneratedMethodAccessor6.invoke(Unknown Source) java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) java.base/java.lang.reflect.Method.invoke(Method.java:564) com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1713) com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:907) com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:943) com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:957) com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) org.apache.lucene.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:49) org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45) org.apache.lucene.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:48) org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:64) org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:47) com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:368) com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:817) com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:468) com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:916) com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:802) com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:852) com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:863) org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45) com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) org.apache.lucene.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:41) com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40) com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40) com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) org.apache.lucene.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:53) org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:47) org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:64) org.apache.lucene.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:54) com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:368) java.base/java.lang.Thread.run(Thread.java:844)

Tim-Brooks · 2018-07-25T19:23:02Z

plugins/transport-nio/src/test/java/org/elasticsearch/http/nio/HttpReadWriteHandlerTests.java

+            assertEquals(HttpRequest.HttpVersion.HTTP_1_1, nioHttpRequest.protocolVersion());
+            assertEquals(RestRequest.Method.GET, nioHttpRequest.method());
+        } finally {
+            handler.close();


I don't know that we care about closing the handler. It probably does not matter too much, but there should not be any resources hanging around if we properly consume all the requests.

Not closing it triggers:

Created at: io.netty.buffer.AbstractByteBufAllocator.compositeHeapBuffer(AbstractByteBufAllocator.java:207) io.netty.buffer.AbstractByteBufAllocator.compositeBuffer(AbstractByteBufAllocator.java:197) io.netty.handler.codec.ByteToMessageDecoder$2.cumulate(ByteToMessageDecoder.java:122) io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:263) io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362) io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348) io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340) io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1359) io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362) io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348) io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:935) io.netty.channel.embedded.EmbeddedChannel.writeInbound(EmbeddedChannel.java:318) org.elasticsearch.http.nio.NettyAdaptor.read(NettyAdaptor.java:104) org.elasticsearch.http.nio.HttpReadWriteHandler.consumeReads(HttpReadWriteHandler.java:81) org.elasticsearch.http.nio.HttpReadWriteHandlerTests.testSuccessfulDecodeHttpRequest(HttpReadWriteHandlerTests.java:129) jdk.internal.reflect.GeneratedMethodAccessor10.invoke(Unknown Source) java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) java.base/java.lang.reflect.Method.invoke(Method.java:564) com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1713) com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:907) com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:943) com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:957) com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) org.apache.lucene.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:49) org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45) org.apache.lucene.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:48) org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:64) org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:47) com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:368) com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:817) com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:468) com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:916) com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:802) com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:852) com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:863) org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45) com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) org.apache.lucene.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:41) com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40) com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40) com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) org.apache.lucene.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:53) org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:47) org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:64) org.apache.lucene.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:54) com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:368) java.base/java.lang.Thread.run(Thread.java:844)

original-brownbear · 2018-07-25T20:34:19Z

@tbrooks8 thanks for taking a look! I reverted the one noop change that snuck in :) Retested out commenting the other two changes you mentioned and reproduced a leak in both cases, so those def. prevent a leak (whether or not that is a problem I can't answer fully, the promise one looks ok to me because we're using a mock channel. The handler one I can't judge though tbh).

Tim-Brooks

LGTM - the adaptor needing to be closed and the promise needing to be completed make sense to me once I look at the stack trace. Those are buffers that netty is creating internally when encoding and decoding http stuff.

original-brownbear · 2018-07-25T20:38:41Z

@tbrooks8 thanks! will merge once green :)

original-brownbear · 2018-07-25T21:54:13Z

Jenkins test this

original-brownbear · 2018-07-25T22:31:02Z

Jenkins test this

@throws

* master: Remove reference to non-existent store type (#32418) [TEST] Mute failing FlushIT test Fix ordering of bootstrap checks in docs (#32417) [TEST] Mute failing InternalEngineTests#testSeqNoAndCheckpoints [TEST] Mute failing testConvertLongHexError bump lucene version after backport Upgrade to Lucene-7.5.0-snapshot-608f0277b0 (#32390) [Kerberos] Avoid vagrant update on precommit (#32416) TESTS: Move netty leak detection to paranoid level (#32354) [DOCS] Fixes formatting of scope object in job resource Copy missing segment attributes in getSegmentInfo (#32396) AbstractQueryTestCase should run without type less often (#28936) INGEST: Fix Deprecation Warning in Script Proc. (#32407) Switch x-pack/plugin to new style Requests (#32327) Docs: Correcting a typo in tophits (#32359) Build: Stop double generating buildSrc pom (#32408) TEST: Avoid triggering merges in FlushIT Fix missing JavaDoc for @throws in several places in KerberosTicketValidator. Switch x-pack full restart to new style Requests (#32294) Release requests in cors handler (#32364) Painless: Clean Up PainlessClass Variables (#32380) Docs: Fix callouts in put license HL REST docs (#32363) [ML] Consistent pattern for strict/lenient parser names (#32399) Update update-settings.asciidoc (#31378) Remove some dead code (#31993) Introduce index store plugins (#32375) Rank-Eval: Reduce scope of an unchecked supression Make sure _forcemerge respects `max_num_segments`. (#32291) TESTS: Fix Buf Leaks in HttpReadWriteHandlerTests (#32377) Only enforce password hashing check if FIPS enabled (#32383)

original-brownbear added >test Issues or PRs that are addressing/adding tests review :Delivery/Build Build or test infrastructure >test-failure Triaged test failures from CI v7.0.0 v6.5.0 labels Jul 25, 2018

original-brownbear requested review from Tim-Brooks and jasontedor July 25, 2018 19:06

original-brownbear mentioned this pull request Jul 25, 2018

[CI] CorsNotSetIT.testThatOmittingCorsHeaderDoesNotReturnAnything fails with ByteBuf leak #32342

Closed

Tim-Brooks reviewed Jul 25, 2018

View reviewed changes

CR: Revert noop release call

8ecc45c

Tim-Brooks approved these changes Jul 25, 2018

View reviewed changes

original-brownbear mentioned this pull request Jul 25, 2018

Release requests in cors handler #32364

Merged

original-brownbear merged commit 48885d2 into elastic:master Jul 26, 2018

original-brownbear deleted the fix-leaks-http-read-write-handler-tests branch July 26, 2018 05:02

original-brownbear removed the v6.5.0 label Jul 26, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

Conversation

original-brownbear commented Jul 25, 2018

Uh oh!

elasticmachine commented Jul 25, 2018

Uh oh!

Tim-Brooks left a comment

Choose a reason for hiding this comment

Uh oh!

Tim-Brooks Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

original-brownbear Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

Tim-Brooks Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

original-brownbear Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

Tim-Brooks Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

original-brownbear Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Jul 25, 2018

Uh oh!

Tim-Brooks left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Jul 25, 2018

Uh oh!

original-brownbear commented Jul 25, 2018

Uh oh!

original-brownbear commented Jul 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants