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

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 catch block which is doing the cleanup part, as this was an existing code? please confirm.

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.

kannanjgithub · 2024-10-17T07:40:40Z

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

@@ -262,25 +268,27 @@ public void deliverFrame(
            if (frame != null) {
              ByteBuf bytebuf = ((NettyWritableBuffer) frame).bytebuf();
              compressionFrame.writeBytes(bytebuf);
+              bytebuf.release();


As per comment #3353 (comment) we should not be doing this in the test.

@kannanjgithub As I remember with this change I was able to fix the below issue with grpcFrame() method.
#3353 (comment)

As per my understanding the comment you mentioned above does not apply here as I am not modifying frame after passing it to channelRead(), also I am not doing anything here around the channelRead() method as you mentioned in above comment link. please confirm

What leak does this fix? I tried commenting this out and ran some of the tests that end up calling this method but I didn't observe any leak report.

As per my knowledge the above change fixed the leakage issue in the grpcFrame() method as mentioned in the comment #3353 (comment) also I remember this error will occur if you run the complete server test by 5+ times (sometimes I have seen these issue in 10 time also). if you run individual tests you will not see leakage error.

kannanjgithub · 2024-10-17T07:41:08Z

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

@@ -357,6 +365,7 @@ protected final ByteBuf captureWrite(ChannelHandlerContext ctx) {
    for (ByteBuf buf : captor.getAllValues()) {
      composite.addComponent(buf);
      composite.writerIndex(composite.writerIndex() + buf.readableBytes());
+      buf.clear();


Fixed, it was part of trial and error.

kannanjgithub · 2024-10-17T08:04:55Z

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

@@ -1302,6 +1320,7 @@ public void maxRstCount_exceedsLimit_fails() throws Exception {
    manualSetUp();
    assertThrows(ClosedChannelException.class, () -> rapidReset(maxRstCount + 1));
    assertFalse(channel().isOpen());
+    channel().releaseOutbound();


I don't see any outbound messages in the channel when this is called. Check here and in other places whether it is really necessary.
Also since you also have it in tearDown does it not suffice already?

@kannanjgithub I can see even with tearDown() method the channel().releaseOutbound() entry is required as I have validated the multiple UT's, without this outbound messages are not getting released. please find the below snippet for one of UT which says messages are not released without this entry(commented this entry in line 379) and its printing the count of the same.

@kannanjgithub Fixed. As per our recent discussion, Removed the channel().releaseOutbound() entries in the UT's which are calling the rapidReset() method as its already handled here.

Also Reverted back the channel().releaseOutbound() in other UTs which is not required as its calling the tearDown() after execution of every test and releasing the outbound messages if any

…tHandlerTest

…pidReset

… from client test

… from base and server test

kannanjgithub · 2024-11-05T06:45:21Z

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

      accumulator += wireSize;
    }
    long pingData = handler.flowControlPing().payload();
    channelRead(pingFrame(true, pingData));
+    channel().releaseOutbound();


This can be removed as this tearDown will do it for you.

…ulates() server base test

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

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: Fixed Review point and leakage issue in dataSizeSincePingAccum…

54cc020

…ulates() server base test

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 Oct 17, 2024

vinodhabib Oct 30, 2024

kannanjgithub Oct 17, 2024

vinodhabib Oct 30, 2024

kannanjgithub Oct 17, 2024

vinodhabib Oct 30, 2024 •

edited

Loading

kannanjgithub Nov 5, 2024

vinodhabib Nov 5, 2024 •

edited

Loading

kannanjgithub Oct 17, 2024

vinodhabib Oct 30, 2024

kannanjgithub Oct 17, 2024

vinodhabib Oct 30, 2024 •

edited

Loading

vinodhabib Nov 4, 2024 •

edited

Loading

kannanjgithub Nov 5, 2024

vinodhabib Nov 5, 2024

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

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 Oct 30, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinodhabib Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

vinodhabib Nov 4, 2024 • edited Loading

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

vinodhabib Oct 30, 2024 •

edited

Loading

vinodhabib Nov 5, 2024 •

edited

Loading

vinodhabib Oct 30, 2024 •

edited

Loading

vinodhabib Nov 4, 2024 •

edited

Loading