Wicket
  1. Wicket
  2. WICKET-625

Wicket doesn't clean up properly when hot-deploying; hangs onto Class references.

    Details

      Description

      When you undeploy a webapp, ideally it should go away and its WebAppClassLoader should be garbage collected. There are various reasons this won't happen, but they essentially split into two problems:

      1) The App Server has references to classes in the WebAppClassLoader in its own objects (on Tomcat these are typically commons logging statics in StandardContext in catalina, or some of the jakarta code). There's not much you can do about this, short of getting a better app server.

      2) You hold references to Class objects loaded by your WebAppClassLoader in static fields in other Classes loaded by your WebAppClassLoader.

      Number 2 can be solved by the use of WeakReferences to the Class objects.

      Note that you also need to be careful about classes that have Class references internally, such as java.lang.reflect.Method and Field. You can also hold these items in a WeakReference, but they have the potential to be garbage collected randomly, unlike the underlying Class objects.

      I have some patches that allow me to start up and shut down a Spring-backed Wicket-based app and have the classloader cope properly. They could probably do with some review.

      1. WICKET-625.patch
        55 kB
        Alastair Maw

        Activity

        Alastair Maw created issue -
        Hide
        Alastair Maw added a comment -

        The attached patch works.
        We need some way to fix the SoftReference stuff better (we should manually clear these caches, I think).

        Show
        Alastair Maw added a comment - The attached patch works. We need some way to fix the SoftReference stuff better (we should manually clear these caches, I think).
        Alastair Maw made changes -
        Field Original Value New Value
        Attachment WICKET-625.patch [ 12359197 ]
        Hide
        Alastair Maw added a comment -

        Note some complexities:
        WeakReference is not Serializable and never will be (http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4217372). I'm therefore replacing current Class references with the equivalent Class.getName(), which should be fine for our purposes.

        Show
        Alastair Maw added a comment - Note some complexities: WeakReference is not Serializable and never will be ( http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4217372 ). I'm therefore replacing current Class references with the equivalent Class.getName(), which should be fine for our purposes.
        Hide
        Johan Compagner added a comment -

        i think we have more class problems that we should solve
        i already reported that once on the dev list: "PackageResource and ResourceReferences keeping a class reference"
        also the DefaultClassResolver is maybe not something we should do..

        Show
        Johan Compagner added a comment - i think we have more class problems that we should solve i already reported that once on the dev list: "PackageResource and ResourceReferences keeping a class reference" also the DefaultClassResolver is maybe not something we should do..
        Hide
        Alastair Maw added a comment -

        I've now put in a bunch of fixes for this, such that we're probably 80% of the way there. This is the easy 80%, mind you.
        The remaining things are harder to fix, as they mostly references Methods not classes.

        Show
        Alastair Maw added a comment - I've now put in a bunch of fixes for this, such that we're probably 80% of the way there. This is the easy 80%, mind you. The remaining things are harder to fix, as they mostly references Methods not classes.
        Hide
        Alastair Maw added a comment -

        Added cache clean-up on per-application basis for PropertyResolver.

        Show
        Alastair Maw added a comment - Added cache clean-up on per-application basis for PropertyResolver.
        Hide
        Igor Vaynberg added a comment -

        here is another thing you might want to look into, if wicket is deployed and undeployed without any url hit you get

        RROR - log - failed wicket
        org.apache.wicket.WicketRuntimeException: There is no application attached to current thread main
        at org.apache.wicket.Application.get(Application.java:170)
        at org.apache.wicket.markup.MarkupCache.<init>(MarkupCache.java:80)
        at org.apache.wicket.settings.Settings.getMarkupCache(Settings.java:1258)
        at org.apache.wicket.Application.internalDestroy(Application.java:827)
        at org.apache.wicket.protocol.http.WebApplication.internalDestroy(WebApplication.java:495)
        at org.apache.wicket.protocol.http.WicketFilter.destroy(WicketFilter.java:104)
        at org.mortbay.jetty.servlet.FilterHolder.doStop(FilterHolder.java:102)
        at org.mortbay.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:65)
        at org.mortbay.jetty.servlet.ServletHandler.doStop(ServletHandler.java:162)
        at org.mortbay.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:65)
        at org.mortbay.jetty.handler.HandlerWrapper.doStop(HandlerWrapper.java:131)
        at org.mortbay.jetty.servlet.SessionHandler.doStop(SessionHandler.java:124)
        at org.mortbay.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:65)
        at org.mortbay.jetty.handler.HandlerWrapper.doStop(HandlerWrapper.java:131)
        at org.mortbay.jetty.handler.ContextHandler.doStop(ContextHandler.java:477)
        at org.mortbay.jetty.webapp.WebAppContext.doStop(WebAppContext.java:513)
        at org.mortbay.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:65)
        at org.mortbay.jetty.handler.HandlerWrapper.doStop(HandlerWrapper.java:131)
        at org.mortbay.jetty.Server.doStop(Server.java:260)
        at org.mortbay.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:65)
        at com.tbs.webapp.StartTbs.main(StartTbs.java:29)

        Show
        Igor Vaynberg added a comment - here is another thing you might want to look into, if wicket is deployed and undeployed without any url hit you get RROR - log - failed wicket org.apache.wicket.WicketRuntimeException: There is no application attached to current thread main at org.apache.wicket.Application.get(Application.java:170) at org.apache.wicket.markup.MarkupCache.<init>(MarkupCache.java:80) at org.apache.wicket.settings.Settings.getMarkupCache(Settings.java:1258) at org.apache.wicket.Application.internalDestroy(Application.java:827) at org.apache.wicket.protocol.http.WebApplication.internalDestroy(WebApplication.java:495) at org.apache.wicket.protocol.http.WicketFilter.destroy(WicketFilter.java:104) at org.mortbay.jetty.servlet.FilterHolder.doStop(FilterHolder.java:102) at org.mortbay.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:65) at org.mortbay.jetty.servlet.ServletHandler.doStop(ServletHandler.java:162) at org.mortbay.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:65) at org.mortbay.jetty.handler.HandlerWrapper.doStop(HandlerWrapper.java:131) at org.mortbay.jetty.servlet.SessionHandler.doStop(SessionHandler.java:124) at org.mortbay.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:65) at org.mortbay.jetty.handler.HandlerWrapper.doStop(HandlerWrapper.java:131) at org.mortbay.jetty.handler.ContextHandler.doStop(ContextHandler.java:477) at org.mortbay.jetty.webapp.WebAppContext.doStop(WebAppContext.java:513) at org.mortbay.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:65) at org.mortbay.jetty.handler.HandlerWrapper.doStop(HandlerWrapper.java:131) at org.mortbay.jetty.Server.doStop(Server.java:260) at org.mortbay.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:65) at com.tbs.webapp.StartTbs.main(StartTbs.java:29)
        Hide
        Alastair Maw added a comment -

        Thanks. Sorted.

        Show
        Alastair Maw added a comment - Thanks. Sorted.
        Hide
        Eelco Hillenius added a comment -

        Assigned version (beta 4)

        Show
        Eelco Hillenius added a comment - Assigned version (beta 4)
        Eelco Hillenius made changes -
        Fix Version/s 1.3.0-beta5 [ 12312818 ]
        Hide
        Thomas Mäder added a comment -

        Just for clarification:

        >2) You hold references to Class objects loaded by your WebAppClassLoader in static fields in other Classes loaded by your WebAppClassLoader.

        why would this keep the WebAppClassLoader from being garbage collected? Is this an implementation detail of the Sun VM or am I missing something? I don't see where the reference is leaked outside the context of the WebAppClassLoader.

        Show
        Thomas Mäder added a comment - Just for clarification: >2) You hold references to Class objects loaded by your WebAppClassLoader in static fields in other Classes loaded by your WebAppClassLoader. why would this keep the WebAppClassLoader from being garbage collected? Is this an implementation detail of the Sun VM or am I missing something? I don't see where the reference is leaked outside the context of the WebAppClassLoader.
        Hide
        Johan Compagner added a comment -

        > why would this keep the WebAppClassLoader from being garbage collected? Is this an implementation detail of the Sun VM or am I missing something? I don't see where the reference is leaked outside the context of the WebAppClassLoader.

        i also don't see that directly, but what could be is that that you give classes that are loaded by the webappclassloader to classes that are from the servlet container itself for example if you have a logging.jar in the server/shared lib. And you give classes of the webapp (not shared) to the logging classes and they keep references then there is a problem.

        Show
        Johan Compagner added a comment - > why would this keep the WebAppClassLoader from being garbage collected? Is this an implementation detail of the Sun VM or am I missing something? I don't see where the reference is leaked outside the context of the WebAppClassLoader. i also don't see that directly, but what could be is that that you give classes that are loaded by the webappclassloader to classes that are from the servlet container itself for example if you have a logging.jar in the server/shared lib. And you give classes of the webapp (not shared) to the logging classes and they keep references then there is a problem.
        Hide
        Alastair Maw added a comment -

        If you keep references to class objects in static fields, the VM isn't capable of deducing that this is a circular reference.

        Method and Field objects internally reference their Class, so you can't hold on to those, either. Equally, you can't put them in a HashMap that's a static field, as it's obviously all the same problem.

        You can easily Google for more information on this if you're not convinced.
        See for example:
        http://www.szegedi.org/articles/memleak.html
        http://opensource.atlassian.com/confluence/spring/pages/viewpage.action?pageId=2669
        etc.

        Show
        Alastair Maw added a comment - If you keep references to class objects in static fields, the VM isn't capable of deducing that this is a circular reference. Method and Field objects internally reference their Class, so you can't hold on to those, either. Equally, you can't put them in a HashMap that's a static field, as it's obviously all the same problem. You can easily Google for more information on this if you're not convinced. See for example: http://www.szegedi.org/articles/memleak.html http://opensource.atlassian.com/confluence/spring/pages/viewpage.action?pageId=2669 etc.
        Hide
        Thomas Mäder added a comment -

        > If you keep references to class objects in static fields, the VM isn't capable of deducing that this is a circular reference.

        That was exactly my question: why can't the VM see the circular reference? Is this specced behaviour or a bug in the Sun VM?

        Show
        Thomas Mäder added a comment - > If you keep references to class objects in static fields, the VM isn't capable of deducing that this is a circular reference. That was exactly my question: why can't the VM see the circular reference? Is this specced behaviour or a bug in the Sun VM?
        Alastair Maw made changes -
        Fix Version/s 1.3.1 [ 12312500 ]
        Fix Version/s 1.3.0-beta5 [ 12312818 ]
        Frank Bille Jensen made changes -
        Fix Version/s 1.3.2 [ 12312942 ]
        Fix Version/s 1.3.1 [ 12312500 ]
        Frank Bille Jensen made changes -
        Fix Version/s 1.3.2 [ 12312942 ]
        Fix Version/s 1.3.3 [ 12313047 ]
        Frank Bille Jensen made changes -
        Fix Version/s 1.3.3 [ 12313047 ]
        Fix Version/s 1.3.4 [ 12313089 ]
        Martijn Dashorst made changes -
        Fix Version/s 1.3.4 [ 12313089 ]
        Fix Version/s 1.3.5 [ 12313175 ]
        Hide
        Igor Vaynberg added a comment -

        marking this as resolved. i believe we have removed all class references...

        Show
        Igor Vaynberg added a comment - marking this as resolved. i believe we have removed all class references...
        Igor Vaynberg made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Resolved [ 5 ]
        Hide
        Daniel Stoch added a comment - - edited

        I think the problem still exists in Injector class (in 1.4), so this issue should be reopened. There is classToFields field with "FIXME: WICKET-625..." comment , which is used to cache fields to inject for specified class. This map is not cleaned up when hot-deploying.
        The patch attached above removes this map (cache), but it has not been applied and I think it is ok, because removing this cache could drastically degrade performance.

        My proposal is to use a solution described in this article: http://weblogs.java.net/blog/jhook/archive/2006/12/class_metadata.html
        and define classToFields field as a:
        WeakHashMap<ClassLoader, ConcurrentHashMap<String, ClassMetaData>>
        where a ClassLoader key will be a class loader of injected object's class. When we will use ClassLoader as the key, then the second map can store class names instead of Class<?> references.

        Show
        Daniel Stoch added a comment - - edited I think the problem still exists in Injector class (in 1.4), so this issue should be reopened. There is classToFields field with "FIXME: WICKET-625 ..." comment , which is used to cache fields to inject for specified class. This map is not cleaned up when hot-deploying. The patch attached above removes this map (cache), but it has not been applied and I think it is ok, because removing this cache could drastically degrade performance. My proposal is to use a solution described in this article: http://weblogs.java.net/blog/jhook/archive/2006/12/class_metadata.html and define classToFields field as a: WeakHashMap<ClassLoader, ConcurrentHashMap<String, ClassMetaData>> where a ClassLoader key will be a class loader of injected object's class. When we will use ClassLoader as the key, then the second map can store class names instead of Class<?> references.
        Hide
        Juergen Donnerstag added a comment -

        see previous comment. Applies to 1.4 as well

        Show
        Juergen Donnerstag added a comment - see previous comment. Applies to 1.4 as well
        Juergen Donnerstag made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Assignee Alastair Maw [ almaw ]
        Juergen Donnerstag made changes -
        Fix Version/s 1.3.5 [ 12313175 ]
        Affects Version/s 1.4.0 [ 12314093 ]
        Igor Vaynberg made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Assignee Igor Vaynberg [ ivaynberg ]
        Fix Version/s 1.4.4 [ 12314323 ]
        Fix Version/s 1.5-M1 [ 12313078 ]
        Resolution Fixed [ 1 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        477d 14h 39m 1 Igor Vaynberg 27/Sep/08 07:31
        Resolved Resolved Reopened Reopened
        307d 9h 34m 1 Juergen Donnerstag 31/Jul/09 17:06
        Reopened Reopened Resolved Resolved
        87d 14h 36m 1 Igor Vaynberg 27/Oct/09 06:43

          People

          • Assignee:
            Igor Vaynberg
            Reporter:
            Alastair Maw
          • Votes:
            4 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development