Improve discovery of (and fix some) netty-releated test buffer leaks #3176
Conversation
This changeset improves disovery of netty-related buffer leaks if the --warn flag is enabled on the command line when running the gradle tests. Paranoid is enabled all the time now, but the overhead of doing so is very minimal and does not need to be enabled conditionally. --warn is needed since only in that mode stdout is visible on the command line. Running it showed a couple of leaks in tests where two of them are already fixed as part of this commit.
| @@ -233,6 +233,11 @@ final class ServiceTalkLibraryPlugin extends ServiceTalkCorePlugin { | |||
| // Always require native libraries for running tests. This helps to make sure transport-level state machine | |||
| // receives all expected network events from Netty. | |||
| systemProperty "io.servicetalk.transport.netty.requireNativeLibs", "true" | |||
|
|
|||
| // Make sure we enable netty leak detection for our tests. By default these are not visible in the logs, | |||
| // to see them you must at --warn to your ./gradlew test run in order to see the "showStandardStreams". | |||
| @@ -252,6 +251,8 @@ private void verifyResponse(String requestPath) { | |||
| assertThat("Unexpected response meta-data", metaData.toString(US_ASCII), containsString(requestPath)); | |||
| ByteBuf payloadBody = channel.readOutbound(); | |||
| assertThat("Unexpected response payload body", payloadBody.toString(US_ASCII), equalTo(RESPONSE_PAYLOAD_BODY)); | |||
| metaData.release(); | |||
| payloadBody.release(); | |||
There was a problem hiding this comment.
I think these should be in a finally block bcz if assertions throw, your new lines won't be invoked
|
|
||
| // Make sure we enable netty leak detection for our tests. By default these are not visible in the logs, | ||
| // to see them you must at --warn to your ./gradlew test run in order to see the "showStandardStreams". | ||
| systemProperty "io.netty.leakDetection.level", "PARANOID" |
There was a problem hiding this comment.
One of the main goals is to check for leaks automatically via ci-prb.yml pipeline. There we can not simply add --warn because it will result in extremely long unreadable build output. We reduced events down to [FAILED] because of that.
We should find a way to always be PARANOID on CI with output to standard streams without changing events, while local development should be more like opt-in for leak detection output as a way to troubleshoot when CI finds something.
One potential idea is to change showStandardStreams logic to be a check on io.netty.leakDetection.level. If it's PARANOID, then it results to true. And then add a system property in ci-prb.yml. Maybe there is other ways as well.
This changeset improves discovery of netty-related buffer leaks if the --warn flag is enabled on the command line when running the gradle tests. Paranoid is enabled all the time now, but the overhead of doing so is very minimal and does not need to be enabled conditionally. --warn is needed since only in that mode stdout is visible on the command line.
Running it showed a couple of leaks in tests where two of them are already fixed as part of this commit.