Hibernate ORM
  1. Hibernate ORM
  2. HHH-2481

Big memory leak in the use of CGLIB

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 3.2.2
    • Fix Version/s: 3.2.3
    • Component/s: core
    • Labels:
      None
    • Environment:
      Hibernate 3.2.2
      MySQL 5.1
    • Bug Testcase Reminder (view):

      Bug reports should generally be accompanied by a test case!

    • Last commented by a user?:
      true

      Description

      The way CGLIBLazyInitializer is creating proxies is resulting in a potentially massive memory leak.

      In CGLIBLazyInitializer.getProxy() just before the proxy is instantiated, a call is made to Enhancer.registerCallbacks() passing in the instance of CGLIBLazyInitializer that will manage the proxy. This variable is stored in a static ThreadLocal on the CGLIB created persistentClass so that any subsequent objects instantiated will get this callback class.

      The problem is that once this ThreadLocal is set, it is never cleared, so it will stay around (together with the object it's managing, and whatever object graph it may be connected to) until the next time a proxy is created for that type on that thread.

      For our application we have about 150 different proxy types, and our app can have over 100 threads. This results in potentially 15,000 proxy objects and their graphs stuck in memory.

      The fix for this is simple. Just make another call the Enhancer.registerCallbacks() with a null callback arg, right after the proxy class is instantiated:

      Enhancer.registerCallbacks(factory, null);

      1. fix.diff
        7 kB
        Paul Malolepsy

        Issue Links

          Activity

          Hide
          Scott Marlow added a comment -

          My next step is to evaluate whether we could fix the root issue differently (or work around it), that being HHH-1293. In the original code, we passed the instance in directly which didn't always work on some platforms (especially Linux). At the very least, we should clear the ThreadLocalStorage reference before returning as your patch does.

          In your example above, is the potential of the leak really that high? I wouldn't expect 15,000 objects leaked. Correct me if I'm wrong, but I would expect one TLS slot per thread to hold a reference. If you have 100 threads, that is a max of 100 objects leaked. Unless a collection is stored in each TLS slot and then the potential is higher. Either way, we should fix the leak.

          http://forum.hibernate.org/viewtopic.php?t=947902 seems to discuss this issue as well.

          Show
          Scott Marlow added a comment - My next step is to evaluate whether we could fix the root issue differently (or work around it), that being HHH-1293 . In the original code, we passed the instance in directly which didn't always work on some platforms (especially Linux). At the very least, we should clear the ThreadLocalStorage reference before returning as your patch does. In your example above, is the potential of the leak really that high? I wouldn't expect 15,000 objects leaked. Correct me if I'm wrong, but I would expect one TLS slot per thread to hold a reference. If you have 100 threads, that is a max of 100 objects leaked. Unless a collection is stored in each TLS slot and then the potential is higher. Either way, we should fix the leak. http://forum.hibernate.org/viewtopic.php?t=947902 seems to discuss this issue as well.
          Hide
          Paul Malolepsy added a comment -

          Yep, that thread discusses this issue exactly.

          I'm afraid the leak is as large as I stated. The static thread local is created on each proxy type. So if my application has a Customer class and a Order class, each type has their own static thread local, and the last proxy object created of each type on each thread will be leaked.

          RE: fixing the issue differently. That occurred to me too. I though you might just be able to call one of the Enhancer.create() methods which let you pass in the callback directly, but I figured there must have been a reason why that wasn't chosen (sounds like I was right) so I decided to respect the original design. But one little extra call to clear the thread local should be a pretty minimal impact.

          Show
          Paul Malolepsy added a comment - Yep, that thread discusses this issue exactly. I'm afraid the leak is as large as I stated. The static thread local is created on each proxy type. So if my application has a Customer class and a Order class, each type has their own static thread local, and the last proxy object created of each type on each thread will be leaked. RE: fixing the issue differently. That occurred to me too. I though you might just be able to call one of the Enhancer.create() methods which let you pass in the callback directly, but I figured there must have been a reason why that wasn't chosen (sounds like I was right) so I decided to respect the original design. But one little extra call to clear the thread local should be a pretty minimal impact.
          Hide
          Scott Marlow added a comment -

          The fix looks good (except we use hard tabs instead of spaces but no big deal).

          We used to pass in the callback directly until we hit HHH-1293. Registering the callback, works around HHH-1293, but not clearing the TLS caused this Jira.

          I'll check the fix into trunk and the 3_2 branch soon.

          Show
          Scott Marlow added a comment - The fix looks good (except we use hard tabs instead of spaces but no big deal). We used to pass in the callback directly until we hit HHH-1293 . Registering the callback, works around HHH-1293 , but not clearing the TLS caused this Jira. I'll check the fix into trunk and the 3_2 branch soon.
          Hide
          Scott Marlow added a comment -

          Fix checked into 3_2 branch and trunk.

          Show
          Scott Marlow added a comment - Fix checked into 3_2 branch and trunk.
          Hide
          Steve Ebersole added a comment -

          Bulk closing stale resolved issues

          Show
          Steve Ebersole added a comment - Bulk closing stale resolved issues

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 5m
                5m
                Remaining:
                Remaining Estimate - 5m
                5m
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development