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

…pidReset

… from client test

… from base and server test

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

…ulates() server base test

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

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.

I had allowed that based on this existing call inside a loop, which gave me the impression that the queue might be getting emptied without releasing and refilled multiple times during the test but that is not the case.

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

Part of grpc#3353

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

vinodhabib added 6 commits October 30, 2024 12:35

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

vinodhabib added 6 commits November 13, 2024 07:43

netty: Fixed review points

65399e8

netty: Fixed review points

0265c68

netty: Reverting back the UTF_8 import change

ea11932

netty: Reverting back the UTF_8 import change

8a61e6f

netty: Replaced \015 with \r to fix the checkstyle issue

5814bb7

netty: Fixed review point by using pre-existing UTF_8 static import

da6595d

vinodhabib requested a review from ejona86 December 2, 2024 09:41

ejona86 approved these changes Dec 2, 2024

View reviewed changes

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Dec 2, 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 Dec 2, 2024

ejona86 merged commit f66d7fc into grpc:master Dec 2, 2024
15 checks passed

larry-safran pushed a commit to larry-safran/grpc-java that referenced this pull request Dec 6, 2024

netty: Fix ByteBuf leaks in tests (grpc#11593)

930bc1a

Part of grpc#3353

github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

netty: netty tests resource leakage issue #11593

netty: netty tests resource leakage issue #11593

Uh oh!

vinodhabib commented Oct 3, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

ejona86 Nov 11, 2024

Uh oh!

vinodhabib Nov 12, 2024 •

edited

Loading

Uh oh!

ejona86 Nov 12, 2024

Uh oh!

vinodhabib Nov 13, 2024

Uh oh!

kannanjgithub Nov 15, 2024

Uh oh!

Uh oh!

ejona86 Nov 11, 2024

Uh oh!

Uh oh!

Uh oh!

netty: netty tests resource leakage issue #11593

netty: netty tests resource leakage issue #11593

Uh oh!

Conversation

vinodhabib commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ejona86 Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

vinodhabib Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejona86 Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

vinodhabib Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

kannanjgithub Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ejona86 Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vinodhabib commented Oct 3, 2024 •

edited

Loading

vinodhabib Nov 12, 2024 •

edited

Loading