jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.83k stars 1.91k forks source link

WebAppClassloader holds jar file handles in WEB-INF/lib #1425

Closed gaohoward closed 4 years ago

gaohoward commented 7 years ago

When using WebAppContext to deploy a war whose WEB-INF/lib has some jar files the war will be extracted to a tmp dir. It appears that those jar files in the tmp dir cannot be automatically cleaned up when the server is shutdown. This happens specifically on Windows platform. Further digging around I found that this relates to certain ClassLoader behaviors that can cause leaked handle (Open inputstream) to those jar files. i.e. the class loader opened an inputstream on a jar file but never closed.

This can cause problem when using jetty as an embedded web server. Each restart of the server will leave some jars in the web app's temp dir. Over time it can occupy a fair amount of disk spaces.

To demonstrate this I have a small test case where I configured an embedded jetty server and deployed a simple war. When I stopped the server the webapp tmp dir has all other files deleted but the jar files. Even if after the test exits VM you can still see those jars in that dir.

I'm working on Apache Artemis and it uses an embedded server to host various web apps. This issue may actually root in JVM however it would be very helpful if this problem can be solved in jetty side.

In my test I also have a customized class loader that manually closes JarFile resources before server stop. This actually make the test pass. It may not be a valid fix but it helps show the problem actually exists.

Please see my attachment. It's a maven project. Just run 'mvn test' it will show you the failure. Note the test passes on Linux as Linux allows a file to be deleted even if there are some one holding the references (handles) to it.

Thanks jetty-simple-test.zip

janbartel commented 7 years ago

@gaohoward this is a known problem on windows. Here's the jetty doco about it, it should give you some hints: https://www.eclipse.org/jetty/documentation/9.3.x/troubleshooting-locked-files-on-windows.html

gaohoward commented 7 years ago

@janbartel Thanks you. but I don't think this is the same issue. It is about clean up of web app's tmp dir. What is expected of jetty server is that when server is stopped, all the contents of the war's temp dir (where the war is exploded into) should be deleted, including static files and binaries like jars. This is supported by WebAppContext via its _persistTmpDir attribute. When it sets to false (default), it uses File.deleteOnExit() to make sure when VM exits those files are deleted automatically. However, because the jar files in web-info/lib are loaded by separate class loaders and didn't get released on stopping, they cannot be deleted on Windows platform.

Btw I modified the web.xml in my war to disable mapped file the test still fails.

janbartel commented 7 years ago

Could be also related to jvm bug reported in jetty bug #575 .

gaohoward commented 7 years ago

@janbartel it sounds familiar but probably a separate issue. The test fails with latest jetty build (9.4.x). To be honest I don't see where WebAppContext misuses the class loader. I've seen some discussions on the web about Java's treatment of JarFile inputstreams but I don't see any confirmation that it should be a bug.

janbartel commented 7 years ago

@gaohoward if you look at WebAppContext, around line 1467 you'll see that when the context is being stopped, we call close() on the webapp's classloader. Can you use a debugger or a debug println to confirm that without your fix, this method is still being called by jetty?

gaohoward commented 7 years ago

@janbartel I can confirm that the said close() method is called correctly. From my point of view the WebAppContext always closes the classloader correctly. Yet somehow there are leaks. I do feel this happens inside URLClassLoader, but I'm not sure what exactly it is. I think maybe this link tells some truth: https://bugs.openjdk.java.net/browse/JDK-8075235

janbartel commented 7 years ago

@gaohoward I think Sun/Oracle really snookered themselves here: when the UrlClassloader calls close on JarUrlConnections, if caching is being used, it has to ignore the close! In other words, there's no way to tell the JarUrlConnection that it really should close and release the cached connection. The only way I can see on Windows is to not use caching with JarUrlConnections and pay any performance penalty. Let me look at the workaround code you posted and other workarounds on the net and see if there is some way they could be optionally invoked by jetty.

janbartel commented 7 years ago

@gaohoward I'm attaching a modified version of jetty's OneWebapp class from examples directory - can you run it on windows and see if it works? You might need to modify the location and name of the webapp to deploy. OneWebApp.zip I've created a LifeCycleListener that listens to the start/stop of the webapp context, and calls your code to do the cleanup of the urls from the WebAppClassLoader.

If this works, then this could be a technique that we could document for how to deal with this problem. Alternatively, you could submit the FixLoaderListener code as a pull request - we'd need to be sure of the provenance of the code that is doing all the reflection, so you'd need to sign the jetty CLA.

Anyway, first step is for you to test.

gaohoward commented 7 years ago

@janbartel Thank you. I'll test it and let you know.

gaohoward commented 7 years ago

@janbartel Hi, I tried your example and unfortunately it doesn't work as expected. When the program exits the jars still remains in the web's tmp dir. It's a bit strange that the code basically the same but my example test works and yours not. I couldn't figure out where could be the problem thou.

Thanks Howard

janbartel commented 7 years ago

@gaohoward it would be great if you could look into it and try and figure out the difference - unfortunately I don't have a windows environment, only *nix.

gaohoward commented 7 years ago

@janbartel I'll try.

gaohoward commented 7 years ago

@janbartel Hi Jan, I found that the cleanup code need to happen before each webContext stop, otherwise it's not working. I have prepared a patch for this and windows test is passing. Can you help review it? Thanks. Here is the link: https://github.com/gaohoward/jetty.project/commit/f5b86c8f13b12aba7120647ee29d82a514146f9b

janbartel commented 7 years ago

@gaohoward so is the trick to making it work to ensure that the jar clearing code is called before the WebAppClassLoader is closed? I'd still like to be able to do this with a LifeCycleListener if possible, so it's easily pluggable.

gaohoward commented 7 years ago

@janbartel that turns out to be the case. Especially this should happen before WebInfConfiguration.deconfigure() method where it attempts to delete files in the tmp dir befor the tmp dir itself deleted on exit. I'm not sure how to achieve this using this LifeCycleListener, of which the execution order seems not guaranteed. Do you have any suggestion?

gregw commented 7 years ago

The sequence in stopping a context is as follows:

  1. WebAppContext#doStop
    1. ContextHandler#doStop
    2. WebAppContext#stopContext
      1. WebAppContext#stopWebapp
        1. ContextHandler#stopContext
          1. ServletContextListener#contextDestroyed
            1. AbstractHandler#doStop
          2. Configuration#deconfigure
        2. URLClassLoader#close

So the context destroyed event on listeners is called before deconfigure on the configurations and before classloader is closed.

On 3 May 2017 at 02:33, Howard Gao notifications@github.com wrote:

@janbartel https://github.com/janbartel that turns out to be the case. Especially this should happen before WebInfConfiguration.deconfigure() method where it attempts to delete files in the tmp dir befor the tmp dir itself deleted on exit. I'm not sure how to achieve this using this LifeCycleListener, of which the execution order seems not guaranteed. Do you have any suggestion?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/eclipse/jetty.project/issues/1425#issuecomment-298798346, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEUrdDwvkLQ3zMGRBnNIWUaQU-dHxfrks5r18ttgaJpZM4Mn3JJ .

-- Greg Wilkins gregw@webtide.com CTO http://webtide.com

gaohoward commented 7 years ago

@gregw Thanks for the good information. Jan assembled the patch code as a LifeCycleListener, it need to be installed so that it runs before each webapp's deconfigure(). How can I achieve this?

gregw commented 7 years ago

So as a LifeCycleListener on the WebAppContext, it is called as follows:

  1. LifeCycleListener#lifeCycleStopping
  2. WebAppContext#doStop
  3. ContextHandler#doStop
    1. WebAppContext#stopContext
      1. WebAppContext#stopWebapp
        1. ContextHandler#stopContext
          1. ServletContextListener#contextDestroyed
          2. AbstractHandler#doStop
        2. Configuration#deconfigure
      2. URLClassLoader#close
    2. LifeCycleListener#lifeCycleStopped

So the stopped part of the listener is indeed called after the close.

So perhaps do all the clean up in the stopping callback? But then the classloader may be closed a little too early.

Perhaps convert the a ServletContextListener, but that may still be a bit early and may compete with other SClisteners.

Actually the best place for it is perhaps in a Configuration#deconfigure as that is the last pluggable place before a close.

cheers

On 3 May 2017 at 08:26, Howard Gao notifications@github.com wrote:

@gregw https://github.com/gregw Thanks for the good information. Jan assembled the patch code as a LifeCycleListener, it need to be installed so that it runs before each webapp's deconfigure(). How can I achieve this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eclipse/jetty.project/issues/1425#issuecomment-298831982, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEUrV2Fb6-YJ30796wyijsR2QjHDGgZks5r2B38gaJpZM4Mn3JJ .

-- Greg Wilkins gregw@webtide.com CTO http://webtide.com

janbartel commented 7 years ago

@gaohoward could you try something else for me please? The problem could simply be that we call WebInfConfiguration.deconfigure() and delete the tmp dir before we call WebAppClassLoder.close(). Could you change WebAppContext.stopContext() method to call WebAppClassLoader.close() at line 1472 before calling all the deconfigure methods? Let me know if that works.

gaohoward commented 7 years ago

@janbartel sure I'll try. Thanks.

gaohoward commented 7 years ago

@janbartel Hi Jan I just tried closing the loader before deconfgiure() method. It doesn't work. All the jars in WEB-INF/lib of each webapp are not deleted. Here is the real code I tried:

 protected void stopContext() throws Exception
 {
     stopWebapp();

Howard

janbartel commented 7 years ago

Thanks for testing that. Curioser and curioser. Let me ponder that for a bit and get back to you.

kornys commented 6 years ago

Any progress with this issue?

joakime commented 6 years ago

@gaohoward any "trick" or "hack" that involves inspecting the URLClassLoader internals via reflection will not on Java 9+ (for many reasons, such as class access restrictions, differences in the internals of URLClassLoader, and even actual classloaders implementations that are not URLClassLoaders).

My personal ultimate hope is to eliminate (on the WebAppContext and WebAppClassloader side) the reliance on URLClassLoader by using the newer zipfs features of Java 8+ (with a bonus of actually supporting JEP238 jars properly!) on a FileSystemClassLoader. (Issue #2386 started this effort, along with the Work-In-Progress PR #2411)

gaohoward commented 6 years ago

Thanks @joakime, I agree with you.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has been a full year without activit. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 4 years ago

This issue has been closed due to it having no activity.