HttpComponents HttpClient
  1. HttpComponents HttpClient
  2. HTTPCLIENT-841

potential memory leak when using ThreadSafeClientConnManager

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.0 Beta 2
    • Fix Version/s: 4.0 Final
    • Component/s: HttpClient
    • Labels:
      None
    • Environment:
      Leopard 10.5.6, Java 1.5.0_16, Jetty 6.1.7

      Description

      When using ThreadSafeClientConnManager and developing with Jetty using auto-redeploy feature eventually I run into a PermGen out of memory exception. I investigated with YourKit 8.0.6 and found a class loader circular reference in RefQueueWorker. Not really sure what I was doing I made the refQueueHandler non-final and nulled it in the shutdown method of RedQueueWorker. I don't seem to have the problem any longer with circular class loader references.

      Here is a diff from 4.0-beta2

      — httpclient/src/main/java/org/apache/http/impl/conn/tsccm/RefQueueWorker.jav(revision 763223)
      +++ httpclient/src/main/java/org/apache/http/impl/conn/tsccm/RefQueueWorker.jav(working copy)
      @@ -50,7 +50,7 @@
      protected final ReferenceQueue<?> refQueue;

      /** The handler for the references found. */

      • protected final RefQueueHandler refHandler;
        + protected RefQueueHandler refHandler;

      /**
      @@ -112,6 +112,8 @@
      this.workerThread = null; // indicate shutdown
      wt.interrupt();
      }
      +
      + refHandler = null;
      }

      1. HTTPCLIENT-841.patch
        27 kB
        Oleg Kalnichevski
      2. HTTPCLIENT-841-depr.patch
        21 kB
        Oleg Kalnichevski
      3. hs_err_pid21886.log
        21 kB
        Olivier Lamy (*$^¨%`£)

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        58d 16h 37m 1 Oleg Kalnichevski 13/Jun/09 11:21
        Resolved Resolved Closed Closed
        591d 38m 1 Oleg Kalnichevski 25/Jan/11 10:59
        Mark Thomas made changes -
        Workflow jira [ 12580828 ] Default workflow, editable Closed status [ 12606511 ]
        Mark Thomas made changes -
        Workflow Default workflow, editable Closed status [ 12557895 ] jira [ 12580828 ]
        Mark Thomas made changes -
        Workflow jira [ 12460877 ] Default workflow, editable Closed status [ 12557895 ]
        Oleg Kalnichevski made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Oleg Kalnichevski added a comment -

        Olivier,

        Yes, I am pretty certain HTTPCLIENT-841as been fixed. I have taken a cursory look at the jvm dump and so far I see no evidence of a memory leak or HttpClient being the cause of the JVM crash. JVM process died unexpectedly, that is for sure, but the reasons appears to be an internal JVM bug

        1. Internal Error (sharedRuntime.cpp:461), pid=21886, tid=20
        2. Error: guarantee(false,"missing exception handler")
        Show
        Oleg Kalnichevski added a comment - Olivier, Yes, I am pretty certain HTTPCLIENT-841 as been fixed. I have taken a cursory look at the jvm dump and so far I see no evidence of a memory leak or HttpClient being the cause of the JVM crash. JVM process died unexpectedly, that is for sure, but the reasons appears to be an internal JVM bug — Internal Error (sharedRuntime.cpp:461), pid=21886, tid=20 Error: guarantee(false,"missing exception handler") —
        Olivier Lamy (*$^¨%`£) made changes -
        Attachment hs_err_pid21886.log [ 12419308 ]
        Hide
        Olivier Lamy (*$^¨%`£) added a comment -

        So personnaly I have some issues with currently.
        Is it really fixed ?
        I will attached a jvm error pid.

        Show
        Olivier Lamy (*$^¨%`£) added a comment - So personnaly I have some issues with currently. Is it really fixed ? I will attached a jvm error pid.
        Oleg Kalnichevski made changes -
        Fix Version/s 4.0 Final [ 12311094 ]
        Fix Version/s 4.0 Beta 3 [ 12313590 ]
        Oleg Kalnichevski made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Oleg Kalnichevski added a comment -

        Patch checked in. Please re-test

        Oleg

        Show
        Oleg Kalnichevski added a comment - Patch checked in. Please re-test Oleg
        Oleg Kalnichevski made changes -
        Attachment HTTPCLIENT-841-depr.patch [ 12410295 ]
        Hide
        Oleg Kalnichevski added a comment -

        Similar patch but retaining 100% API compatibility. If I hear no objections, and no one steps in to fix the connection GC code, I'll commit this patch in a few days and close the issue.

        Oleg

        Show
        Oleg Kalnichevski added a comment - Similar patch but retaining 100% API compatibility. If I hear no objections, and no one steps in to fix the connection GC code, I'll commit this patch in a few days and close the issue. Oleg
        Oleg Kalnichevski made changes -
        Attachment HTTPCLIENT-841.patch [ 12409994 ]
        Hide
        Oleg Kalnichevski added a comment -

        Folks

        I took a look at this issue and this is what I have to say. With Roland gone I do not see a way of coming up with a reliable fix for the problem without removing / disabling the connection garbage collection code. Moreover, I can't help thinking this feature causes more harm than good. It basically makes ThreadSafeClientConnManager, well, not thread safe. Personally I am prepared to go as far as breaking the API compatibility to have a better peace of mind about reliability of ThreadSafeClientConnManager and to simplify a fairly complex code, which all this GC magic makes even more complex.

        I'll once again try to get in touch with Google folks to find out how they feel about possibility of breaking API / binary compatibility with Android.

        I am attaching a patch that completely rips out garbage collection of connections.

        Oleg

        Show
        Oleg Kalnichevski added a comment - Folks I took a look at this issue and this is what I have to say. With Roland gone I do not see a way of coming up with a reliable fix for the problem without removing / disabling the connection garbage collection code. Moreover, I can't help thinking this feature causes more harm than good. It basically makes ThreadSafeClientConnManager, well, not thread safe. Personally I am prepared to go as far as breaking the API compatibility to have a better peace of mind about reliability of ThreadSafeClientConnManager and to simplify a fairly complex code, which all this GC magic makes even more complex. I'll once again try to get in touch with Google folks to find out how they feel about possibility of breaking API / binary compatibility with Android. I am attaching a patch that completely rips out garbage collection of connections. Oleg
        Hide
        Sebb added a comment -

        If the feature was made optional - e.g. via a parameter as hinted at in the code - then the default could be to not include it.

        Show
        Sebb added a comment - If the feature was made optional - e.g. via a parameter as hinted at in the code - then the default could be to not include it.
        Hide
        Oleg Kalnichevski added a comment -

        Sam,

        By all of means please do take a stab at it. However, personally I would prefer to be on the cautious side and exclude this feature for the 4.0 release anyway.

        Oleg

        Show
        Oleg Kalnichevski added a comment - Sam, By all of means please do take a stab at it. However, personally I would prefer to be on the cautious side and exclude this feature for the 4.0 release anyway. Oleg
        Hide
        Sebb added a comment -

        The implementation is currently difficult to make thread-safe, so removing it would be good.

        If redesigned, it would be good to remove/rework the enableConnectionGC() method, which can only be called immediately after creating the pool.
        The method creates additional instance objects (refQueue and refWorker); if these could be optionally created at construction time they could be made final to improve thread-safety. There would still need to be a way to start the RefQueueWorker thread after the AbstractConnPool was created, but this could be done by the concrete subclass constructor (as is done now).

        Show
        Sebb added a comment - The implementation is currently difficult to make thread-safe, so removing it would be good. If redesigned, it would be good to remove/rework the enableConnectionGC() method, which can only be called immediately after creating the pool. The method creates additional instance objects (refQueue and refWorker); if these could be optionally created at construction time they could be made final to improve thread-safety. There would still need to be a way to start the RefQueueWorker thread after the AbstractConnPool was created, but this could be done by the concrete subclass constructor (as is done now).
        Hide
        Sam Berlin added a comment -

        I have a little familiarity with it. I'll take a look – I think the garbage collection closing is a useful thing to keep, if possible.

        Show
        Sam Berlin added a comment - I have a little familiarity with it. I'll take a look – I think the garbage collection closing is a useful thing to keep, if possible.
        Hide
        Oleg Kalnichevski added a comment -

        This code was originally written by Roland and I suspect he never managed to get around to finishing it completely. I am a bit uneasy about touching component no one seems to have good knowledge of and making changes no one seems to be completely sure about.

        I am leaning towards just disabling garbage collection of connections for the 4.0 release and revisiting this problem in the course of 4.1 development.

        Any objections to that?

        Oleg

        Show
        Oleg Kalnichevski added a comment - This code was originally written by Roland and I suspect he never managed to get around to finishing it completely. I am a bit uneasy about touching component no one seems to have good knowledge of and making changes no one seems to be completely sure about. I am leaning towards just disabling garbage collection of connections for the 4.0 release and revisiting this problem in the course of 4.1 development. Any objections to that? Oleg
        Hide
        Sebb added a comment -

        This may be completely off-base, but:

        Is it possible that the RefQueueWorker shutdown() method was called directly, rather than using the appropriate pool shutdown() method(s)?

        [Maybe the RefQueueWorker shutdown() method should be package scope?]

        Might be worth adding some debug code to check whether or not the refHandler resources have all been released.
        Not sure where to put this though, as RefQueueWorker.shutdown() is called first.

        Show
        Sebb added a comment - This may be completely off-base, but: Is it possible that the RefQueueWorker shutdown() method was called directly, rather than using the appropriate pool shutdown() method(s)? [Maybe the RefQueueWorker shutdown() method should be package scope?] Might be worth adding some debug code to check whether or not the refHandler resources have all been released. Not sure where to put this though, as RefQueueWorker.shutdown() is called first.
        Oleg Kalnichevski made changes -
        Field Original Value New Value
        Fix Version/s 4.0 Beta 3 [ 12313590 ]
        Hide
        Oleg Kalnichevski added a comment -

        Bizarre. Anyone has an idea why this should matter?

        Oleg

        Show
        Oleg Kalnichevski added a comment - Bizarre. Anyone has an idea why this should matter? Oleg
        Ted Slusser created issue -

          People

          • Assignee:
            Unassigned
            Reporter:
            Ted Slusser
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development