Uploaded image for project: 'Pig'
  1. Pig
  2. PIG-4603

Classloader leak in SpillableMemoryManager

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Won't Fix
    • Affects Version/s: 0.15.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      In the constructor of SpillableMemoryManager, a NotificationListener is added to the MemoryMXBean. This listener is never removed, even if the application is re-deployed.

      This results in the instance SpillableMemoryManager being still reachable after a redeploy. This results in its class being reachable, which results in the classloader being reachable, along with all the classes in it.

      This leads to metaspace OutOfMemoryErrors.

        Activity

        Hide
        rohini Rohini Palaniswamy added a comment -

        Gleb Smirnov,
        This is a good find. Do you want to work on this? If not I can take it up.

        Show
        rohini Rohini Palaniswamy added a comment - Gleb Smirnov , This is a good find. Do you want to work on this? If not I can take it up.
        Hide
        gvsmirnov Gleb Smirnov added a comment -

        Rohini Palaniswamy, I am not even remotely familiar with the codebase, so it would probably take me some huge amount of time to fix this. The issue itself was auto-detected. So it would be better if someone closer to the project (e.g. you ) would take it.

        Show
        gvsmirnov Gleb Smirnov added a comment - Rohini Palaniswamy , I am not even remotely familiar with the codebase, so it would probably take me some huge amount of time to fix this. The issue itself was auto-detected. So it would be better if someone closer to the project (e.g. you ) would take it.
        Hide
        rohini Rohini Palaniswamy added a comment -

        No problem. We just offer the reporter of the issue the first chance to fix it as well . Did you find the issue with Tez container reuse?

        Show
        rohini Rohini Palaniswamy added a comment - No problem. We just offer the reporter of the issue the first chance to fix it as well . Did you find the issue with Tez container reuse?
        Hide
        gvsmirnov Gleb Smirnov added a comment -

        The issue was found by Plumbr. One of our customers is running Pig, and faced this problem on a redeploy.

        Show
        gvsmirnov Gleb Smirnov added a comment - The issue was found by Plumbr. One of our customers is running Pig, and faced this problem on a redeploy.
        Hide
        rohini Rohini Palaniswamy added a comment -

        I realized that it cannot be a problem with Tez container reuse. Can you clarify "redeploy" ? Is it on the pig client side? Are they deploying Pig in some webapp and hence the multiple classloaders?

        Show
        rohini Rohini Palaniswamy added a comment - I realized that it cannot be a problem with Tez container reuse. Can you clarify "redeploy" ? Is it on the pig client side? Are they deploying Pig in some webapp and hence the multiple classloaders?
        Hide
        gvsmirnov Gleb Smirnov added a comment -

        Yep, Pig is deployed into Tomcat 7.0.54

        Show
        gvsmirnov Gleb Smirnov added a comment - Yep, Pig is deployed into Tomcat 7.0.54
        Hide
        rohini Rohini Palaniswamy added a comment -

        I could not find an easy fix for this.

        NotificationEmitter.removeNotificationListener(NotificationListener listener) requires the exact object to be passed to it. If it allowed registering of listeners with a unique id/name in addition to the object, it would have been easy to remove the older one before registering the new one. Even otherwise it is a bad idea if more than one live webapp has to be supported.

        The clean solution would be to implement ServletContextListener.contextDestroyed and call
        ((NotificationEmitter)ManagementFactory.getMemoryMXBean()).removeNotificationListener(SpillableMemoryManager.getInstance(), null, null); in that by whoever is packaging pig as a webapp. Since that is outside of the scope of Pig, I am closing this as Won't Fix.

        Show
        rohini Rohini Palaniswamy added a comment - I could not find an easy fix for this. NotificationEmitter.removeNotificationListener(NotificationListener listener) requires the exact object to be passed to it. If it allowed registering of listeners with a unique id/name in addition to the object, it would have been easy to remove the older one before registering the new one. Even otherwise it is a bad idea if more than one live webapp has to be supported. The clean solution would be to implement ServletContextListener.contextDestroyed and call ((NotificationEmitter)ManagementFactory.getMemoryMXBean()).removeNotificationListener(SpillableMemoryManager.getInstance(), null, null); in that by whoever is packaging pig as a webapp. Since that is outside of the scope of Pig, I am closing this as Won't Fix.
        Hide
        gvsmirnov Gleb Smirnov added a comment -

        Fair point here. A simple solution that comes to mind would be to have another class that is subscribed for notifications and holds a weak reference to the SpillableMemoryManager. All the notifications are delegated there. As soon as the SpillableMemoryManager itself becomes weakly reachable, removeNotificationListener may be invoked. This is somewhat not elegant, but would save the skin of people who use Pig inside of an application server.

        Show
        gvsmirnov Gleb Smirnov added a comment - Fair point here. A simple solution that comes to mind would be to have another class that is subscribed for notifications and holds a weak reference to the SpillableMemoryManager. All the notifications are delegated there. As soon as the SpillableMemoryManager itself becomes weakly reachable, removeNotificationListener may be invoked. This is somewhat not elegant, but would save the skin of people who use Pig inside of an application server.
        Hide
        mate Mattias Jiderhamn added a comment -

        I have implemented a workaround (i.e. removeNotificationListener() for any NotificationListener that may cause leaks) for this in my ClassLoader Leak Prevention library. I would be thankful if any users of Pig would care to build the library from GitHub sources and test the workaround, before I make a release including this fix.

        Show
        mate Mattias Jiderhamn added a comment - I have implemented a workaround (i.e. removeNotificationListener() for any NotificationListener that may cause leaks) for this in my ClassLoader Leak Prevention library . I would be thankful if any users of Pig would care to build the library from GitHub sources and test the workaround, before I make a release including this fix.

          People

          • Assignee:
            rohini Rohini Palaniswamy
            Reporter:
            gvsmirnov Gleb Smirnov
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development