reactor / reactor-core

Non-Blocking Reactive Foundation for the JVM
http://projectreactor.io
Apache License 2.0
4.99k stars 1.21k forks source link

Fix memoryleak in FluxOnAssembly #3941

Open travishaagen opened 12 hours ago

travishaagen commented 12 hours ago

This PR contains one possible fix for a memory leak discovered in FluxOnAssembly.

My colleague, @foo4u discovered a memory leak in one of our applications, and I believe that it is caused by some code in FluxOnAssembly. The nodesPerId HashMap expects the key to be derived from calling hashCode() on an object, but inside FluxOnAssembly.OnAssemblyException#add(Publisher<?>, Publisher<?>, String, String) the debugger is showing that an AssemblyOp implementation is the object type. None of these implementations implement hashCode(), so we're getting the integer returned by Object.hashCode() instead.

What I'm observing in a Kotlin Spring Webflux project is that every time we make a HTTP request to a Controller, this code path is being hit three times, which results in three objects being added to the HashMap. Indirectly, this code path is being triggered by some code in https://github.com/DataDog/dd-trace-java

FluxOnAssembly hasn't been changed in three years, and to be honest, the nodesPerId and how it is used in this class is odd. It might only be used to increment a counter, but it might also be used as a single point of concurrency control for a couple blocks of code.

All the AssemblyOp implementations override toString() with something that looks unique, and was probably what the original author intended to base the nodesPerId key on.

Here is an example of three keys, from this PR, attributed to a single HTTP request:

checkpoint("Handler com.xyz.MyController#myFunc(String, String, String, String, String, ServerWebExchange, Continuation) [DispatcherHandler]")

checkpoint("com.xyz.ShutdownWebFilter [DefaultWebFilterChain]")

checkpoint("HTTP POST "/endpoint" [ExceptionHandlingWebHandler]")

Willing to discuss!

Also! There are some other uses of System.identityHashCode in the codebase, but I don't have insights into them at the moment.