hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sam Berlin" <sber...@gmail.com>
Subject Re: Connection Manager Garbage Collection
Date Tue, 24 Jul 2007 04:35:36 GMT
I am not fully aware of the situation surrounding the CM, but I would
like to point out one problem that may be an issue with HttpNIO.  If
the connection's socket is registered with a Selector and the socket
is set to keep-alive on the TCP-level with the server, there is
nothing that will ever cause that Socket to get garbage collected.  It
never leaves the scope of any active object, and everything else the
Selector has stored (which includes the attachment in the
SelectionKey) remains strongly-referenced, too.  The attachment
probably contains references to the rest of connection's path, as it
should.  Any attempt to weakly reference these objects is perilous, as
part of the point of the Reactor pattern is that it only responds to
events, so it makes very much sense to have the only strong reference
from the SelectionKey's attachment.

Basically, this means that any attempt to leave stray connection
cleanups to the garbage collector will likely fail miserably in the
NIO variant.

(On a related note, I am not positive on what happens if the Socket
gets closed but interest is off for reading & writing.  It may or may
not signal some kind of event.)

Sam

On 7/23/07, Michael Becke <mbecke@gmail.com> wrote:
> 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
>
>

---------------------------------------------------------------------
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