Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve discovery of (and fix some) netty-releated test buffer leaks #3176

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Improve discovery of (and fix some) netty-releated test buffer leaks #3176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...nal/src/main/groovy/io/servicetalk/gradle/plugin/internal/ServiceTalkLibraryPlugin.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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".
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at -> add?

systemProperty "io.netty.leakDetection.level", "PARANOID"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

systemProperty "io.netty.leakDetection.targetRecords", "25"
}

dependencies {
Expand Down
2 changes: 1 addition & 1 deletion ...http-netty/src/test/java/io/servicetalk/http/netty/H2PriorKnowledgeFeatureParityTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2070,7 +2070,7 @@ public void channelReadComplete(ChannelHandlerContext ctx) {
}

private static void onDataRead(ChannelHandlerContext ctx, Http2DataFrame data) {
ctx.write(new DefaultHttp2DataFrame(data.content().retainedDuplicate(), data.isEndStream()));
ctx.write(new DefaultHttp2DataFrame(data.content(), data.isEndStream()));
}

private void onHeadersRead(ChannelHandlerContext ctx, Http2HeadersFrame headers) {
Expand Down
3 changes: 2 additions & 1 deletion ...etalk-http-netty/src/test/java/io/servicetalk/http/netty/ServerRespondsOnClosingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@
import static org.hamcrest.Matchers.is;

class ServerRespondsOnClosingTest {

private static final HttpResponseFactory RESPONSE_FACTORY = new DefaultHttpResponseFactory(
DefaultHttpHeadersFactory.INSTANCE, DEFAULT_ALLOCATOR, HTTP_1_1);
private static final String RESPONSE_PAYLOAD_BODY = "Hello World";
Expand Down Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these should be in a finally block bcz if assertions throw, your new lines won't be invoked

}

private void respondWithFIN() throws Exception {
Expand Down
Loading