hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Becke" <mbe...@gmail.com>
Subject Re: Connection Manager Garbage Collection
Date Tue, 24 Jul 2007 03:41:51 GMT
Hello,

More comments below.

> > Currently there is nothing that closes threads other than shutdown.
>
> I was referring to the connections, but if shutdown isn't
> called, then the pooled connections will just be left to
> the GC and the built-in finalizer for open sockets, right?

Guess so.  Assuming sockets do something in finalize.  Ideally
connection managers should always be shutdown though.  I see
finalizers as an unreliable last resort.

> > Not sure if we'd want to put this in a finalizer or not.  In general I
> > have always avoided using them but perhaps this would be a good use
> > case.
>
> I have an idea how to handle this from the background thread.
> But I'll put something into finalizers too, because I want to
> make connection GC, and thereby the background thread, optional.

Will be interested in seeing it.

> > I have also attached changes to the LostConnWorker that should
> > alleviate the issue with the GC test case.
>
> Unfortunately, the patch introduces threading (=GC) issues:
>
>    while (this.workerThread == Thread.currentThread() &&
>           connPoolRef.get() != null) {
>        try {
>             //@@@ (1)
>             Reference ref = ((ConnectionPool) connPoolRef.get()
>                              ).refQueue.remove();
>             //@@@ (2)
>             if (ref instanceof PoolEntryRef) {
>               ((ConnectionPool) connPoolRef.get()
>                ).handleReference((PoolEntryRef) ref);
>             }
>        } catch (InterruptedException e) {
>          LOG.debug("LostConnWorker interrupted", e);
>        }
>    }
>
> If GC runs at one of the positions marked @@@, then the
> following dereferencing of the weak reference may trigger
> a NullPointerException. The operation between (1) and (2)
> is actually blocking until the GC puts something into the
> reference queue, so that is the most likely point of failure.
> One might argue that the NPX is thrown only when the pool
> is GCed and the thread should die anyway, but I feel that
> would be a bit too crude.

Yes, was assuming at worst there would be a NPE which could be
ignored.  Probably not the best.  Should be easy to work around though
if necessary.

> > remove all of the code related to the
> > ReferenceQueueThread.  After that is done the only static code left is
> > related to ALL_CONNECTION_MANAGERS.  Is the plan to keep or remove
> > this code?
>
> It needs to go. As long as we have shared, static maps in the
> code, it's not safe for use in shared environments such as
> OSGi (Felix, Eclipse) or servlet containers (when put in a
> parent classloader).

Quite right.

> So here is my dubious stratagem...
>
> 1. move TSCCM into a separate package
> 2. factor out some of the inner classes
>   a) replace the nesting with weak or hard references
>   b) introduce interfaces where needed
> 3. move static maps into a separate class which
>    is marked for extinction
> 4. document the relations between the classes
>    in a package.html
> 5. perform remaining cleanup, exterminate 3.
> 6. see what can be generalized
>
> (not sure about the order of 4 and 5)

Yes, either order should be fine.  Could really skip three and go
straight to 5 if you like.

Not sure how much of TSCCM can be generalized.  Will be nice to
separate things out, but I wouldn't put too much effort into
generalization unless we really think there's a case for reuse or
substitution.

Mike

---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org


Mime
View raw message