Default value of spring.netty.leak-detection does not match Netty's default

[lfz757077613]

I use netty to build a tcp server from springboot 2.4.x to 2.5.x and I found the project does not detect memory leak any more.
The NettyAutoConfiguration was added from 2.5.x, and the LeakDetection of NettyProperties is disable.
I think it shoud keep same with netty default ResourceLeakDetector level, otherwise the netty debug log shows its ResourceLeakDetector level is simple, but it is disable actually.
In most situations, we need the default simple ResourceLeakDetector and it works well
By the way, NettyAutoConfiguration seems useless for now, because it only set ResourceLeakDetector. Maybe it will provide more properties in the future？

[pivotal-cla]

@lfz757077613 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@lfz757077613 Thank you for signing the Contributor License Agreement!

[snicoll]

Thanks for the suggestion. It looks like either the default has changed or the default we've set in #14338 was the wrong one.

[bclozel]

We've set it to the wrong level originally. I'm scheduling this for 2.6.0.

[lfz757077613]

I agree with it 100%. At least for me, I choose to exclude NettyAutoConfiguration unless it can provide more properties.

[bclozel]

@lfz757077613 Thanks for raising this issue. I'm not sure I understand your concern here - besides working around this particular issue (to be fixed in 2.5.2), why would you want to exclude this auto-configuration?

[lfz757077613]

@lfz757077613 Thanks for raising this issue. I'm not sure I understand your concern here - besides working around this particular issue (to be fixed in 2.5.2), why would you want to exclude this auto-configuration?

Like you commented on 27 oct 2018, it effect "everything netty". The jvm argument maybe the best practice for the global setting such like it.
And NettyAutoConfiguration make users who both use springboot and netty confused, such as me.
When I found my project did not detect memory leak any more, I checked the netty debug log at the first time. It really make me confused when I saw the log like below. Maybe it's better to add a log in NettyAutoConfiguration

- - 2021-06-17 23:44:33.928 DEBUG 17072 --- [           main] io.netty.util.ResourceLeakDetector       : -Dio.netty.leakDetection.level: simple
- - 2021-06-17 23:44:33.928 DEBUG 17072 --- [           main] io.netty.util.ResourceLeakDetector       : -Dio.netty.leakDetection.targetRecords: 4
- - 2021-06-17 23:44:33.933 DEBUG 17072 --- [           main] i.n.util.ResourceLeakDetectorFactory     : Loaded default ResourceLeakDetector: io.netty.util.ResourceLeakDetector@7e91ed74

Even through I unify the ResourceLeakDetector level setting by using NettyAutoConfiguration, I also need to consider the lifecircle between spring container and other component which also set ResourceLeakDetector level. So I prefer to control the ResourceLeakDetector level by myself.
But I'd like to use it if NettyAutoConfiguration can provide other settings like BufAllocator, etc in the future
Thanks for reading my large text above 😅

[bclozel]

Thanks for your comment @lfz757077613 - so you were previously setting that value yourself and you were surprised that Spring Boot is setting it as well.

Maybe we should change the current situation so that we don't override a non-default value? Or maybe we should set a null value as a default and only change it if the developer provides one?

Maybe we could add logging statements using the same logger to show that the level has been changed by the application?

As for additional properties, we might add new ones if the community makes a good case for them.

[lfz757077613]

Thanks for your comment @lfz757077613 - so you were previously setting that value yourself and you were surprised that Spring Boot is setting it as well.

Maybe we should change the current situation so that we don't override a non-default value? Or maybe we should set a null value as a default and only change it if the developer provides one?

Maybe we could add logging statements using the same logger to show that the level has been changed by the application?

As for additional properties, we might add new ones if the community makes a good case for them.

I think default "null" is a good idea, I have modify this pr and add test in NettyAutoConfigurationTests and fix additional-spring-configuration-metadata.json

[bclozel]

Thanks for your contribution @lfz757077613 (both the PR and the discussion). We've discussed this matter and chose to go in a different direction. Because we'd like to ship this change with the next release tomorrow, we're closing this PR and addressing that in #27046.

A few things to consider:

1. In Default value for NettyProperties.leakDetection is not aligned with Netty's default #27046, we'll align our configuration property with the default value in Netty and cover that with a unit test
2. We understand that the logging behavior can be confusing, but currently this is only logging changes done at the static initialization of the class at the Netty level, so really logging the system property set. If anyone is changing that by calling ResourceLeakDetector.setLevel(level) statically, nothing will be logged. If you think this is a problem, you can voice your opinion in the Netty issue tracker and discuss this matter. Personally, I think the behavior is consistent still, because it's only surfacing system properties.
3. We think that other options (null default value, additional logging) aren't helping here and would not align with the rest of Spring Boot's configuration properties behavior.

I'm closing this PR as a result. Thanks!

[lfz757077613]

@bclozel Hi, I think additional-spring-configuration-metadata.json is forgot to change the defaultValue in 2.5.2

[bclozel]

@lfz757077613 Thanks, I've opened #27104 to fix that.