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
Netty isn't being shut down properly? #29
Comments
@alan-czajkowski Can you provide some more details about your setup? How are you shutting down Tomcat (via an IDE, command-line, etc.), and how and when are you calling |
I have a vanilla deployment of Apache Tomcat 7.0.47 on a Ubuntu 12 machine. I'm starting Pushy via a Spring managed bean:
and my bean is being properly destroyed by Spring:
|
In my own testing, it looks like the global event executor goes away several hundred milliseconds after the |
I put in a Thread.sleep(2000) right after the pushManager.shutdown() line, which helped. No more SEVERE reporting in the Tomcat logs. |
Great! Seems like that global event executor doesn't exist until we're shutting down, and then lingers. Will have to do some digging to figure that one out. |
@alan-czajkowski I've opened an upstream bug report for this issue: netty/netty#2084 |
Looks like that upstream issue isn't getting much attention right now (which is understandable because it's not a very severe problem and it's a busy time of the year in much of the world). If it's not resolved/acknowledged/etc. by the end of the coming week, though, I'll assume that the |
thanks |
shall we re-open this issue to incorporate the recommended solution mentioned in netty/netty#2084 ? |
Sure. |
So having taken a look at the options, I don't think this is a thing we want to do in It wouldn't make sense to wait for the global event executor to shut down if other things are still using it, so I think this is a decision best left to callers. Seems like the best thing to do is to further update the wiki/README and let callers manage Netty shutdown on their own. |
can a different shutdown method be introduced? cleanShutdown() ? which incorporates the Netty recommended solution? you can JavaDoc/README the disclaimer on using the method :) |
Well, that would still require callers to be aware of the issue and make the same decision about how to shut down, but would also remove some transparency and flexibility. Since the Netty-recommended solution is only two lines long, I don't think the benefit of encapsulating it outweighs the cost of having another shutdown path in terms of clarity, flexibility, or testing. Maybe I'm missing the point, though. Why do you think it would be better to have the waiting happen inside a Pushy shutdown method? |
... because Netty is an implementation detail within Pushy. Asking a user to perform Netty operations outside of Pushy is not ideal. What if Netty stops being a dependency of Pushy? the code can potentially break? I know this is not an easy decision. Is there some way to use Netty in an isolated way to only shut down the Netty resources that Pushy uses? I'm not an expert in Netty, that is why I'm asking. |
After some research and communication with the Netty folks, I'm convinced that there's no viable, generally-applicable technical solution to be had here. What works in one case may make things worse in another. I think the best we can do is document the issue and let users decide what's right for their case. Beyond that, we're going to change our official recommendation to "don't use Pushy in a servlet container if you're worried about leaks at shutdown." |
This issue requires more investigation, but here is a brief summary:
I have a Java web app running on Apache Tomcat 7.0.47 using JDK 1.7.45.
When you shut down Tomcat, the following errors appear:
SEVERE: The web application [] appears to have started a thread named [globalEventExecutor-1-2] but has failed to stop it. This is very likely to create a memory leak.
I know that the GlobalEventExecutor thread is created by Netty.
When Pushy is being shut down via PushManager.shutdown(), is Netty (and other resources) being shut down properly as well?
The text was updated successfully, but these errors were encountered: