Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.0
    • Fix Version/s: 2.2.0
    • Component/s: None
    • Labels:
      None
    • Environment:

      Glassfish 4, windows 7

      Description

      but during troubleshooting a classloader-leak seemingly related to jersey and com.fasterxml.jackson, I found the following class-loader leak:

      Inside org.jvnet.hk2.internal.Utilities class we find a methodKeyCache field, which is a WeakHashMap.
      The key is a Class<?> (and so far so good), but the value is HashSet of MemberKey, where backingMember is a method instance, but which (in this case unfortunately), contains a hard-reference to a Class<?> instance (which happens to be the same as the key).

      This seems therefore a clear violation of the guideline described in the docs:
      Implementation note: The value objects in a WeakHashMap are held by ordinary strong references. Thus care should be taken to ensure that value objects do not strongly refer to their own keys, either directly or indirectly, since that will prevent the keys from being discarded.

        Activity

        Hide
        jwells added a comment -

        I talked to some people on the Java team today about this and turns out that WeakReferences aren't truly what we want here, instead we need to have SoftReferences. The trouble with WeakReferences is that they will go out of the cache very quickly and hence is leading to unexpected results. SoftReferences will only go out of scope if there is memory pressure on the JVM. The code seems to work now with SoftReferences, so I'll check that in.

        Show
        jwells added a comment - I talked to some people on the Java team today about this and turns out that WeakReferences aren't truly what we want here, instead we need to have SoftReferences. The trouble with WeakReferences is that they will go out of the cache very quickly and hence is leading to unexpected results. SoftReferences will only go out of scope if there is memory pressure on the JVM. The code seems to work now with SoftReferences, so I'll check that in.
        Hide
        notabenem added a comment -

        Sounds good. So that would mean that in case of multiple re-deployments, we could see several instances of the classloader for a specific app, but those should not result in "PermGen" problems. Sounds good.

        So what about the 2.1.88 hotfix? Do you see that realistic?

        Show
        notabenem added a comment - Sounds good. So that would mean that in case of multiple re-deployments, we could see several instances of the classloader for a specific app, but those should not result in "PermGen" problems. Sounds good. So what about the 2.1.88 hotfix? Do you see that realistic?
        Hide
        jwells added a comment -

        So there were problems in GlassFish even using SoftReferences. I am working on another fix, but have found a few other places where Method was being used as a key in a cache (see ReflectionHelper accessibleMethodCache for an example).

        Show
        jwells added a comment - So there were problems in GlassFish even using SoftReferences. I am working on another fix, but have found a few other places where Method was being used as a key in a cache (see ReflectionHelper accessibleMethodCache for an example).
        Hide
        jwells added a comment -

        Wow. Getting rid of this classloader leak was tough. The final fix involves PhantomReferences and SoftReferences and changes to 5 different caches. There still may be more, I'm going to do an inventory of the code to find others. However, I think the current solution removes the leak, is fairly efficient in the normal case and does not have the cache grow without bound (by removing removed classes when the PhantomReference notices classes go).

        I have even added a new System variable to turn off the SoftReference lookup that is used by the unit test code in order to get higher code coverage. This variable is:

        org.jvnet.hk2.properties.useSoftReference

        It defaults to "true," which does make a big performance difference.

        Show
        jwells added a comment - Wow. Getting rid of this classloader leak was tough. The final fix involves PhantomReferences and SoftReferences and changes to 5 different caches. There still may be more, I'm going to do an inventory of the code to find others. However, I think the current solution removes the leak, is fairly efficient in the normal case and does not have the cache grow without bound (by removing removed classes when the PhantomReference notices classes go). I have even added a new System variable to turn off the SoftReference lookup that is used by the unit test code in order to get higher code coverage. This variable is: org.jvnet.hk2.properties.useSoftReference It defaults to "true," which does make a big performance difference.
        Hide
        jwells added a comment -

        Fixed, though it has not been backported.

        Show
        jwells added a comment - Fixed, though it has not been backported.

          People

          • Assignee:
            jwells
            Reporter:
            notabenem
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: