glassfish
  1. glassfish
  2. GLASSFISH-17142

ClassLoader / memory leak in ConnectorTimerProxy

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 3.1.1_b12
    • Fix Version/s: None
    • Component/s: jca
    • Labels:
      None

      Description

      The ConnectorTimerProxy is used to lazily initialize a Timer object. There once was an issue where usage of this proxy caused the proxied timer thread to be bound to the WebApp's ClassLoader, thus preventing it from being discarded when undeploying the application (see GLASSFISH-7074).

      Unfortunately, there is another way to create a memory leak: ConnectorTimerProxy extends java.util.Timer and thus it has a TimerThread of its own that's never used since all methods delegate to the proxy timer.
      BUT this means that the very first call to getProxy() will create a TimerThread that inherits the calling thread's context class loader and it seems I just came across this case.

      jhat says:

      Static reference from com.sun.enterprise.connectors.util.ConnectorTimerProxy.connectorTimer
      (from class com.sun.enterprise.connectors.util.ConnectorTimerProxy) :
      --> com.sun.enterprise.connectors.util.ConnectorTimerProxy@0x9c036b8 (28 bytes) (field thread:)
      --> java.util.TimerThread@0x9bf1e58 (113 bytes) (field contextClassLoader:)
      --> org.glassfish.web.loader.WebappClassLoader@0x97d9630 (164 bytes) 
      

      Since the ConnectorTimerProxy's own TimerThread is not used at all, I'd suggest to add a call to super.cancel() in the constructor.

        Activity

        Hide
        rwruck added a comment -

        Update 2: It seems that the current workaround is not enough to eliminate references to the WebApp ClassLoader. While the contextClassLoader of the timer threads is now set to the connectorClassLoader, they still inherit the caller thread's AccessControlContext - which in turn refers to the thread's ClassLoader.

        jhat says:

        Static reference from com.sun.enterprise.connectors.util.ConnectorTimerProxy.connectorTimer
        (from class com.sun.enterprise.connectors.util.ConnectorTimerProxy) :
        --> com.sun.enterprise.connectors.util.ConnectorTimerProxy@0x9b66f00 (32 bytes) (field timer:)
        --> java.util.Timer@0x9b746e0 (20 bytes) (field thread:)
        --> java.util.TimerThread@0x9b55748 (113 bytes) (field inheritedAccessControlContext:)
        --> java.security.AccessControlContext@0x9b72308 (21 bytes) (field context:)
        --> [Ljava.security.ProtectionDomain;@0x9b7c358 (80 bytes) (Element 6 of [Ljava.security.ProtectionDomain;@0x9b7c358:)
        --> java.security.ProtectionDomain@0x983e028 (30 bytes) (field classloader:)
        --> org.glassfish.web.loader.WebappClassLoader@0x9762c30 (164 bytes)
        

        I don't know of any way to specify a newly created thread's AccessControlContext - in fact, being able to do so would break the whole security model.

        Maybe I'm missing something but to me it seems that the ConnectorTimerProxy should NEVER be used by a thread whose AccessControlContext might refer to a WebApp's ProtectionDomain - or at least getProxy() and getTimer() need to be called once before any WebApp has a chance to do so. That would reduce the problem to cases where handleTimerException is involved.

        Show
        rwruck added a comment - Update 2: It seems that the current workaround is not enough to eliminate references to the WebApp ClassLoader. While the contextClassLoader of the timer threads is now set to the connectorClassLoader, they still inherit the caller thread's AccessControlContext - which in turn refers to the thread's ClassLoader. jhat says: Static reference from com.sun.enterprise.connectors.util.ConnectorTimerProxy.connectorTimer (from class com.sun.enterprise.connectors.util.ConnectorTimerProxy) : --> com.sun.enterprise.connectors.util.ConnectorTimerProxy@0x9b66f00 (32 bytes) (field timer:) --> java.util.Timer@0x9b746e0 (20 bytes) (field thread:) --> java.util.TimerThread@0x9b55748 (113 bytes) (field inheritedAccessControlContext:) --> java.security.AccessControlContext@0x9b72308 (21 bytes) (field context:) --> [Ljava.security.ProtectionDomain;@0x9b7c358 (80 bytes) (Element 6 of [Ljava.security.ProtectionDomain;@0x9b7c358:) --> java.security.ProtectionDomain@0x983e028 (30 bytes) (field classloader:) --> org.glassfish.web.loader.WebappClassLoader@0x9762c30 (164 bytes) I don't know of any way to specify a newly created thread's AccessControlContext - in fact, being able to do so would break the whole security model. Maybe I'm missing something but to me it seems that the ConnectorTimerProxy should NEVER be used by a thread whose AccessControlContext might refer to a WebApp's ProtectionDomain - or at least getProxy() and getTimer() need to be called once before any WebApp has a chance to do so. That would reduce the problem to cases where handleTimerException is involved.
        Hide
        rwruck added a comment -

        Update: While it may be a good idea to stop the unneccesary timer thread, it does not solve the issue, because the TimerThread object which holds the reference to the WebApp's ClassLoader is not set to null in the JDK's cancel() implementation.
        A patched glassfish where the getProxy() method uses the same workaround as the getTimer() method, i.e. use the ConnectorClassLoader for creating the ConnectorTimerProxy instance itself, does not expose the described issue anymore.

        Show
        rwruck added a comment - Update: While it may be a good idea to stop the unneccesary timer thread, it does not solve the issue, because the TimerThread object which holds the reference to the WebApp's ClassLoader is not set to null in the JDK's cancel() implementation. A patched glassfish where the getProxy() method uses the same workaround as the getTimer() method, i.e. use the ConnectorClassLoader for creating the ConnectorTimerProxy instance itself, does not expose the described issue anymore.
        Hide
        Hong Zhang added a comment -

        assign to jagadish for evaluation

        Show
        Hong Zhang added a comment - assign to jagadish for evaluation

          People

          • Assignee:
            Jagadish
            Reporter:
            rwruck
          • Votes:
            6 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated: