-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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.
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
Memory leak when using Jetty ALPN SSL provider #3080
Comments
@ejona86 Wanted to make sure you saw this one. Happy to help if you can point us in the right direction as well. |
@JackOfMostTrades Are you sure that jar works with 131? Looking at https://github.com/jetty-project/jetty-alpn-agent/blob/master/src/main/java/org/mortbay/jetty/alpn/agent/Premain.java#L35 it looks like it only works up to 121 Any reason you aren't using the alpn-agent instead of boot? |
It seems to work fine for me with 131, but regardless I get the same behavior with Oracle JDK 121: No, there's no particular reason for using alpn-boot as opposed to the agent. I also just tried substituting that in as well in the reproducer and I get the same. |
@carl-mastrangelo, it works with 131. I think the 121 is the first compatible version, not the last.
Drat. 😛 It looks like Netty adds the reference in the SSLEngine constructor. It seems like maybe there's some case where the SslHandler doesn't close the handler? gRPC's code looks pretty minimal. But the SslHandler also seems pretty solid. We can also maybe try older gRPC versions and see if it is a semi-recent regression. |
I just went back through versions to see and it looks like the behavior first cropped up in 0.15.0 (it looks like there's no issue with 0.14.1). FWIW that's also when the listener API for the name resolver changed from |
Motivation: When using the JdkSslEngine, the ALPN class is used keep a reference to the engine. In the event that the TCP connection fails, the SSLEngine is not removed from the map, creating a memory leak. Modification: Always close the SSLEngine regardless of if the channel became active. Also, record the SSLEngine was closed in all places. Result: Fixes: grpc/grpc-java#3080
@JackOfMostTrades I sent a PR to fix netty for this, sorry for sitting on it for so long. I really appreciate the reproducer! |
Awesome, thanks for digging in and figuring out where the issue was! |
Motivation: When using the JdkSslEngine, the ALPN class is used keep a reference to the engine. In the event that the TCP connection fails, the SSLEngine is not removed from the map, creating a memory leak. Modification: Always close the SSLEngine regardless of if the channel became active. Also, record the SSLEngine was closed in all places. Result: Fixes: grpc/grpc-java#3080
Please answer these questions before submitting your issue.
What version of gRPC are you using?
1.4.0
What JVM are you using (
java -version
)?openjdk version "1.8.0_131"
OpenJDK Runtime Environment (build 1.8.0_131-8u131-b11-1-b11)
OpenJDK 64-Bit Server VM (build 25.131-b11, mixed mode)
What did you do?
We have observed a memory leak when using the Jetty ALPN SSL provider. This occurs when using a name resolver that returns multiple results, one of which fails to connect (in practice this was because of faulty firewall rules, but for the sake of testing it can be reproduced by just using a bad port number). I believe the managed channel will keep trying to open up a subchannel, but the callbacks in the ALPN.objects map aren't getting cleared.
I've created a minimal reproducer for this here: https://github.com/JackOfMostTrades/memory-leak-reproducer
When left running overnight, the size of the map grew to over 2300 objects (since there's only ever one actual connection in this test, that's pretty clearly an issue).
This may be a bug in the underlying netty channel rather than gRPC's managed channel where it's not properly cleaning up its ALPN callback when this type of error occurs, but I didn't dig deep enough into the issue to be able to tell.
The text was updated successfully, but these errors were encountered: