ActiveMQ
  1. ActiveMQ
  2. AMQ-4182

Memory Leak for ActiveMQBytesMessage with Compression as true

    Details

    • Type: Bug Bug
    • Status: Reopened
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 5.5.1
    • Fix Version/s: None
    • Component/s: JMS client
    • Labels:
      None
    • Environment:

      Linux(Redhat 5.5), Windows 7

      Description

      InflaterInputStream is supposed to close explicitly to release resource allocated by its JNI methods. In ActiveMQBytesMessage, dataIn property is disposed simply without closing it, which results in some weird memory leak that can't be detected from heap size. It can't be controlled by -Xmx or -XX:MaxDirectMemorySize.

      Please run the following test program to verify the issue:

      import java.util.concurrent.TimeUnit;

      import javax.jms.BytesMessage;
      import javax.jms.Connection;
      import javax.jms.Session;

      import org.apache.activemq.ActiveMQConnectionFactory;
      import org.apache.activemq.command.ActiveMQBytesMessage;

      /**

      • A simple test to verify memory leak in ActiveMQBytesMessage.
        */
        public class Main
        {
        public static void main(String[] args) throws Exception
        {
        ActiveMQConnectionFactory connFactory = new ActiveMQConnectionFactory("vm://localhost");
        connFactory.setUseCompression(true);
        Connection conn = connFactory.createConnection();
        Session session = conn.createSession(false, Session.AUTO_ACKNOWLEDGE);
        BytesMessage message = session.createBytesMessage();

      message.writeBytes(new byte[1024]);

      ActiveMQBytesMessage m = (ActiveMQBytesMessage)message;
      if(!m.isCompressed())

      { throw new RuntimeException(); }

      while (true)
      {
      for (int k = 0; k < 1024; ++k)

      { message.reset(); byte[] data = new byte[1024]; message.readBytes(data); }

      TimeUnit.MILLISECONDS.sleep(10);
      }
      }

      }

        Activity

        Hide
        Timothy Bish added a comment -

        Added some additional logic to attempt to close the dataIn stream when possible.

        Show
        Timothy Bish added a comment - Added some additional logic to attempt to close the dataIn stream when possible.
        Hide
        Jeff Huang added a comment -

        I read the changes. Fix in reset() is good I believe. But I am worried about the finalize() method because I don't think it would work as good as we would expect. Please take a look at how Sun's JDK 1.6 implemented java.util.zip.Inflater

        336 /**
        337 * Closes the decompressor and discards any unprocessed input.
        338 * This method should be called when the decompressor is no longer
        339 * being used, but will also be called automatically by the finalize()
        340 * method. Once this method is called, the behavior of the Inflater
        341 * object is undefined.
        342 */
        343 public void end() {
        344 synchronized (zsRef) {
        345 long addr = zsRef.address();
        346 zsRef.clear();
        347 if (addr != 0)

        { 348 end(addr); 349 buf = null; 350 }

        351 }
        352 }
        353
        354 /**
        355 * Closes the decompressor when garbage is collected.
        356 */
        357 protected void finalize()

        { 358 end(); 359 }

        They have already done finalize() with end(). So if finalize() works well, the problem wouldn't exist in the first place. And since those memory is allocated out of JVM heap, the GC could have problem to sense the necessity to clean something even the system blows out.

        Show
        Jeff Huang added a comment - I read the changes. Fix in reset() is good I believe. But I am worried about the finalize() method because I don't think it would work as good as we would expect. Please take a look at how Sun's JDK 1.6 implemented java.util.zip.Inflater 336 /** 337 * Closes the decompressor and discards any unprocessed input. 338 * This method should be called when the decompressor is no longer 339 * being used, but will also be called automatically by the finalize() 340 * method. Once this method is called, the behavior of the Inflater 341 * object is undefined. 342 */ 343 public void end() { 344 synchronized (zsRef) { 345 long addr = zsRef.address(); 346 zsRef.clear(); 347 if (addr != 0) { 348 end(addr); 349 buf = null; 350 } 351 } 352 } 353 354 /** 355 * Closes the decompressor when garbage is collected. 356 */ 357 protected void finalize() { 358 end(); 359 } They have already done finalize() with end(). So if finalize() works well, the problem wouldn't exist in the first place. And since those memory is allocated out of JVM heap, the GC could have problem to sense the necessity to clean something even the system blows out.
        Hide
        Jeff Huang added a comment -

        Please try to run the test program with -Xmx10M, -X128M, and -X2G, and monitor the JVM memory usage. Here are the results in my own test system with 8G memory when it was stable:

        -Xmx10M
        PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
        15070 root 17 0 429m 123m 9856 S 80.2 1.5 0:48.49 java

        -Xmx128M
        PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
        15115 root 17 0 2808m 2.0g 9856 S 78.6 26.0 0:22.70 java

        -X2G
        Never stable and the whole system just ran out of memory.

        That means when -Xmx is big enough for holding NORMAL objects the GC just won't see the memory problem at all, which results in the finalize() method won't be executed ever.

        Show
        Jeff Huang added a comment - Please try to run the test program with -Xmx10M, -X128M, and -X2G, and monitor the JVM memory usage. Here are the results in my own test system with 8G memory when it was stable: -Xmx10M PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND 15070 root 17 0 429m 123m 9856 S 80.2 1.5 0:48.49 java -Xmx128M PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND 15115 root 17 0 2808m 2.0g 9856 S 78.6 26.0 0:22.70 java -X2G Never stable and the whole system just ran out of memory. That means when -Xmx is big enough for holding NORMAL objects the GC just won't see the memory problem at all, which results in the finalize() method won't be executed ever.
        Hide
        Jeff Huang added a comment -

        The change on finalize() won't fix the problem because java.util.zip.Inflater tries that already. The memory problem happens out of the normal heap and GC won't detect the memory shortage, in turn objects won't be wiped out at all.

        Show
        Jeff Huang added a comment - The change on finalize() won't fix the problem because java.util.zip.Inflater tries that already. The memory problem happens out of the normal heap and GC won't detect the memory shortage, in turn objects won't be wiped out at all.
        Hide
        Timothy Bish added a comment -

        You're welcome to submit a patch if you can see a reasonable solution to the issue you are seeing.

        Show
        Timothy Bish added a comment - You're welcome to submit a patch if you can see a reasonable solution to the issue you are seeing.
        Hide
        Jeff Huang added a comment -

        Unfortunately, I don't have a solution for that. I would tend to say that there is not a valid solution for that.

        We could try to add a close() or release() method to ActiveMQBytesMessage, but I don't think the users are going to call it because it is not part of the JMS API.

        Or we can try to find another pure Java compression library to avoid the need to release the resource explicitly. But that may probably mean we need to sacrifice some performance when doing compression.

        Or we may want to decompress the whole buffer, instead of remaining a stream for it. The memory footprint would be bigger, but comparing to blowing out the whole system, this is acceptable.

        Or we can call off this feature. Tell the users not to use BytesMessage in ActiveMQ when they try to use compression.

        Show
        Jeff Huang added a comment - Unfortunately, I don't have a solution for that. I would tend to say that there is not a valid solution for that. We could try to add a close() or release() method to ActiveMQBytesMessage, but I don't think the users are going to call it because it is not part of the JMS API. Or we can try to find another pure Java compression library to avoid the need to release the resource explicitly. But that may probably mean we need to sacrifice some performance when doing compression. Or we may want to decompress the whole buffer, instead of remaining a stream for it. The memory footprint would be bigger, but comparing to blowing out the whole system, this is acceptable. Or we can call off this feature. Tell the users not to use BytesMessage in ActiveMQ when they try to use compression.

          People

          • Assignee:
            Unassigned
            Reporter:
            Jeff Huang
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development