netty: netty tests resource leakage issue #11593

vinodhabib · 2024-10-03T06:09:53Z

I can see there are multiple tests in netty server and client side are randomly failing with resource leakage error and this PR will be fixing some of them at Server and Client side as below.

Note : Made some common method changes (as per Eric Suggestion/Reference) in all 3 tests files and currently tests which are shown below in Pending Server Base are one which are majorly impacting in both Server and Client test randomly which has common root cause while invoking the dataFrame() which we discussed earlier and tried suggestion as shown in the comment
#3353 (comment)

Server Side UTs Fixes:
sendFrameShouldSucceed()
inboundDataWithEndStreamShouldForwardToStreamListener()
inboundDataShouldForwardToStreamListener()
keepAliveEnforcer_sendingDataResetsCounters()
maxRstCount_withinLimit_succeeds()
maxRstCount_exceedsLimit_fails()

NettyHandlerTestBase.dataSizeSincePingAccumulates()

Client Side UTs Fixes
sendFrameShouldSucceed()

Pending UTs at Server Base
NettyHandlerTestBase.bdpPingWindowResizing
NettyHandlerTestBase.bdpPingLimitOutstanding
NettyHandlerTestBase.windowUpdateMatchesTarget
NettyHandlerTestBase.testPingBackoff

Note : you can refer the comment #3353 (comment) for more details on these 4 pending Server Base UTs behavior

Fixes #3353

…ts and commented 3 Failing UTs.

… framer and release buffers

…tenerMessageQueue in NettyServerHandlerTest

# Conflicts: # netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java

…stenerMessageQueue in NettyServerTests as it's not working as expected

kannanjgithub · 2024-10-17T07:26:25Z

netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java

@@ -1346,7 +1368,7 @@ private ByteBuf emptyGrpcFrame(int streamId, boolean endStream) throws Exception
    try {
      return dataFrame(streamId, endStream, buf);
    } finally {
-      buf.release();
+      buf.clear();


Revert this change.

if we revert this change then below tests are failing with mentioned reason in snippet. so I used clear() also you can refer previous comment on the same.
#3353 (comment)

As per documentation clear() does not release memory: https://github.com/netty/netty/blob/64c52255c073d4a308d3dbb56a12d3cca7692c47/buffer/src/main/java/io/netty/buffer/ByteBuf.java#L467

I tried with your PR and not calling either byteBuf.clear() or byteBuf.release() here and I did not observe the leak report for any of the 3 tests calling emptyDataFrame() viz headersWithErrAndEndStreamReturnErrorButNotThrowNpe, streamErrorShouldNotCloseChannel and clientHalfCloseShouldForwardToStreamListener.

I think in the above comment and snippet we are discussing about 3 tests failures with refCnt decrement error if we use byteBuf.release() instead of byteBuf.clear(), not about memory leakage issue

No, my question is why byteBuf.release() is even required when it doesn't show any report leak.

you mean to say we need to remove this try block which is doing the cleanup part, as this was an existing code? please confirm.

No, leave it as is. For some reason I see
minus on buf.release() and
plus on buf.clear()
making it appear like it was your change.

I agree the release() looks like it should go away (the ownership is being transferred to dataFrame(), but clear() is useless here. Just delete the line?

kannanjgithub · 2024-10-17T07:26:33Z

netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java

  /**
   * Must be called by subclasses to initialize the handler and channel.
   */
  protected final void initChannel(Http2HeadersDecoder headersDecoder) throws Exception {
-    content = Unpooled.copiedBuffer("hello world", UTF_8);
+    //ByteBuf content = Unpooled.copiedBuffer("hello world", UTF_8);


Remove commented line.

kannanjgithub · 2024-10-17T07:26:42Z

netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java

@@ -869,7 +873,8 @@ public void keepAliveEnforcer_sendingDataResetsCounters() throws Exception {
    future.get();
    for (int i = 0; i < 10; i++) {
      future = enqueue(
-          new SendGrpcFrameCommand(stream.transportState(), content().retainedSlice(), false));
+          //new SendGrpcFrameCommand(stream.transportState(), content().retainedSlice(), false));


Remove commented line.

netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java

netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java

…tHandlerTest

…pidReset

… from client test

… from base and server test

netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java

…ulates() server base test

ejona86 · 2024-11-11T14:59:36Z

netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java

@@ -16,8 +16,8 @@

 package io.grpc.netty;

+import static com.google.common.base.Charsets.UTF_8;


Why did you make this change?

this was changed as per your earlier suggestion/reference #3353 (comment)

I don't understand. I said nothing about this, and it wasn't in the code change I linked.

@ejona86 Below snippet is captured from code change which you linked where I can see the UTF_8 import is already exists.

FileName - NettyHandlerTestBase.java master...ejona86:grpc-java:netty-leak-detector-in-tests#:~:text=final%20byte%5B%5D-,contentAsArray,-()%20%7B

Note : I removed chars \000\000\000\000\015 from String as its giving check style violation while build

ejona86 · 2024-11-11T20:39:17Z

netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java

@@ -1315,7 +1313,9 @@ private void rapidReset(int burstSize) throws Exception {
    for (int period = 0; period < 3; period++) {
      for (int i = 0; i < burstSize; i++) {
        channelRead(headersFrame(streamId, headers));
+        channel().releaseOutbound();


Why do we need both releaseOutbound()s?

I have added these releaseOutbounds() here as i saw leakage error in the below 2 UTs which are calling the rapidReset() method and it has outbound messages which are not released.

maxRstCount_withinLimit_succeeds()
maxRstCount_exceedsLimit_fails()

releaseOutbound() is the sort of thing that only needs to be run once at the end of tests, to avoid leaks. Running it at the end of @Before could be okay if we don't want tests to see any queued messages, but there's no point to calling it multiple times here. Really, I'm surprised it isn't placed mostly just once in an @After to clean up after every test.

netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java

ejona86 · 2024-11-11T21:28:50Z

netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java

@@ -1346,7 +1368,7 @@ private ByteBuf emptyGrpcFrame(int streamId, boolean endStream) throws Exception
    try {
      return dataFrame(streamId, endStream, buf);
    } finally {
-      buf.release();
+      buf.clear();


I agree the release() looks like it should go away (the ownership is being transferred to dataFrame(), but clear() is useless here. Just delete the line?

…x the UTs

vinodhabib added 25 commits August 5, 2024 19:30

Defect-3353 - Added the Suggestions by Eric with recent issues commen…

07c8b0b

…ts and commented 3 Failing UTs.

Defect-3353 - Added the Suggestions by Eric with recent issues commen…

58ecc92

…ts and commented 3 Failing UTs.

Defect-3353 - Added the Suggestions by Eric with recent issues commen…

74674ea

…ts and commented 3 Failing UTs.

Defect-3353 - Added the Suggestions by Eric with recent issues commen…

2f60656

…ts and commented 3 Failing UTs.

Defect-3353 - Fixed 3 UTs with Leakage issue.

7b4a65a

Defect-3353 - Fixed dataSizeSincePingAccumulates UT case.

ec566f7

Defect-3353 - Fixed Failing 3 UTs

5989fd7

Defect-3353 - Fixed/Modified the maxRstCount related UTs.

148833b

Defect-3353 - Fixed Failing UT in NettyClientHandlerTest

9f9b873

Defect-3353 - Fixed Randomly Failing UTs in Base Test

0018b16

Defect-3353 - Updated to framer.close() method will flush, closes the…

e4d85c5

… framer and release buffers

Defect-3353 - Fixed UT - inboundDataShouldForwardToStreamListener

8530a63

Defect-3353 - Fixed UT - inboundDataShouldForwardToStreamListener

0ec3f2a

Defect-3353 - Fixed UT - windowUpdateMatchesTarget

41ec48e

Merge branch 'master' into defect-3353-nettyTestsResourceLeakFix

fd64d5d

Defect-3353 - Updated rapidReset method

b5cc9c8

Defect-3353 - Removed frame.clear() as per suggestions

0ecd460

Defect-3353 - Fixed the grpcFrame method leakage issue

ff5bd67

Defect-3353 - Added changes to use streamTransportState and streamLis…

01dc433

…tenerMessageQueue in NettyServerHandlerTest

Merge branch 'master' into defect-3353-nettyTestsResourceLeakFix

8376fad

# Conflicts: # netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java

Defect-3353 - Code cleanup

8bfe353

Defect-3353 - Fixed the PR check issue

f316d5e

Defect-3353 - Fixed the PR check issue

1d9b406

Defect-3353 - Fixed the PR check issue

06f1692

Defect-3353 - Fixed checkstyle issues

44197ee

vinodhabib changed the title ~~Defect 3353 - netty tests resource leak fix~~ netty - netty tests resource leakage issue Oct 4, 2024

vinodhabib changed the title ~~netty - netty tests resource leakage issue~~ netty: netty tests resource leakage issue Oct 4, 2024

grpc-netty: Removing changes to use streamTransportState and streamLi…

08590fd

…stenerMessageQueue in NettyServerTests as it's not working as expected

vinodhabib changed the title ~~netty: netty tests resource leakage issue~~ grpc-netty: netty tests resource leakage issue Oct 14, 2024

grpc-netty: Fixed checkstyle issue

454ff6a

Merge branch 'master' into defect-3353-nettyTestsResourceLeakFix

abdbb21

kannanjgithub reviewed Oct 17, 2024

View reviewed changes

netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java Show resolved Hide resolved

kannanjgithub reviewed Oct 17, 2024

View reviewed changes

netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java Outdated Show resolved Hide resolved

kannanjgithub reviewed Oct 17, 2024

View reviewed changes

netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java Outdated Show resolved Hide resolved

vinodhabib changed the title ~~grpc-netty: netty tests resource leakage issue~~ netty: netty tests resource leakage issue Oct 18, 2024

vinodhabib added 8 commits October 30, 2024 06:47

netty: Fixed recent review points

e0ad32b

netty: added releaseOutbound() entry in setup() method for NettyClien…

f0dcb2b

…tHandlerTest

netty: reverted back the change to fix failing test in netty server

31602a6

netty: Fixed leakage issues of bdpPing UTs in Netty Client test

0ae38ee

netty: Removed the releaseOutbound entry as its already handled in ra…

d0e893e

…pidReset

netty: Reverted back releaseOutboundMessage entry which is not needed

bbeb872

netty: Reverted back releaseOutboundMessage entry which is not needed…

d0787e2

… from client test

netty: Reverted back releaseOutboundMessage entry which is not needed…

efbc818

… from base and server test

kannanjgithub reviewed Nov 5, 2024

View reviewed changes

netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java Outdated Show resolved Hide resolved

netty: Fixed Review point and leakage issue in dataSizeSincePingAccum…

54cc020

…ulates() server base test

kannanjgithub approved these changes Nov 11, 2024

View reviewed changes

kannanjgithub added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 11, 2024

kannanjgithub requested a review from ejona86 November 11, 2024 08:17

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 11, 2024

ejona86 reviewed Nov 11, 2024

View reviewed changes

vinodhabib added 2 commits November 12, 2024 07:15

netty: Fixed the recent review points

6c67cea

netty: Reverted back the code which was part of trial and error to fi…

b99debf

…x the UTs

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 12, 2024

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 12, 2024

netty: Fixed review points

65399e8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

netty: netty tests resource leakage issue #11593

netty: netty tests resource leakage issue #11593

vinodhabib commented Oct 3, 2024 •

edited

Loading

kannanjgithub Oct 17, 2024

vinodhabib Oct 30, 2024 •

edited

Loading

kannanjgithub Nov 5, 2024

vinodhabib Nov 5, 2024

kannanjgithub Nov 5, 2024

vinodhabib Nov 5, 2024 •

edited

Loading

kannanjgithub Nov 6, 2024 •

edited

Loading

ejona86 Nov 11, 2024

vinodhabib Nov 12, 2024

kannanjgithub Oct 17, 2024

vinodhabib Oct 30, 2024

kannanjgithub Oct 17, 2024

vinodhabib Oct 30, 2024

ejona86 Nov 11, 2024

vinodhabib Nov 12, 2024 •

edited

Loading

ejona86 Nov 12, 2024

vinodhabib Nov 13, 2024 •

edited

Loading

ejona86 Nov 11, 2024

vinodhabib Nov 12, 2024 •

edited

Loading

ejona86 Nov 12, 2024

vinodhabib Nov 13, 2024

ejona86 Nov 11, 2024

		@@ -16,8 +16,8 @@

		package io.grpc.netty;

		import static com.google.common.base.Charsets.UTF_8;

netty: netty tests resource leakage issue #11593

Are you sure you want to change the base?

netty: netty tests resource leakage issue #11593

Conversation

vinodhabib commented Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

vinodhabib Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinodhabib Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

kannanjgithub Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinodhabib Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinodhabib Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinodhabib Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinodhabib commented Oct 3, 2024 •

edited

Loading

vinodhabib Oct 30, 2024 •

edited

Loading

vinodhabib Nov 5, 2024 •

edited

Loading

kannanjgithub Nov 6, 2024 •

edited

Loading

vinodhabib Nov 12, 2024 •

edited

Loading

vinodhabib Nov 13, 2024 •

edited

Loading

vinodhabib Nov 12, 2024 •

edited

Loading