You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by Roland Weber <os...@dubioso.net> on 2007/07/22 06:31:05 UTC
Connection Manager Garbage Collection
Hi Mike, Oleg, all,
I need some advice on TSCCM garbage collection. I think
I have the connection GC covered, but there's also the
GC of the connection manager itself. Based on some
comments in the code, I believe there are two cases:
1. a connection is allocated. Then the TSCCM will not be
GCed, because an entry in REFERENCE_TO_CONNECTION_SOURCE
prevents it. The entry references the ConnectionPool,
which is a non-static nested class of the TSCCM, and
therefore implicitly references the TSCCM.
2. no connection is allocated. Then the TSCCM is GCed,
we have a unit test for this. But how are the open
connections closed in this scenario? I didn't see a
finalizer that would close them.
I am considering to move the TSCCM into a separate
package o.a.h.i.conn.tsccm, so I can factor out some
TSCCM specific classes without cluttering the main
implementation package. There are way too many nested
classes to feel comfortable with. This would also
create a home for some stuff elsewhere which looks
generic but is really tailored for the TSCCM, such
as the current selection of parameters.
Oleg, I'm glad you asked me to port the unit tests before
starting the refactoring. These things are much easier
to debug one at a time, while I am making the changes.
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: Connection Manager Garbage Collection
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Sat, 2007-08-11 at 20:13 +0200, Roland Weber wrote:
> Roland Weber wrote:
> > 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
>
> I finally got around to take care of 4.
> Please have a look and let me know whether
> the documentation achieves to explain how
> connection garbage collection depends on
> the proper use of hard and weak references:
> http://people.apache.org/~rolandw/client-jdoc/org/apache/http/impl/conn/tsccm/package-summary.html
>
Hi Roland
TSCCM is probably the most complex piece of code in HttpComponents and I
believe you did a good job documenting its inner workings.
Oleg
> Ignoring that there is always more cleanup
> to be done, steps 1-5 are complete. Step 6
> has a low priority. HTTPCLIENT-677 is now
> the top item on the todo list for TSCCM.
>
> cheers,
> Roland
>
> ---------------------------------------------------------------------
> 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
Re: Connection Manager Garbage Collection
Posted by Roland Weber <os...@dubioso.net>.
Roland Weber wrote:
> 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
I finally got around to take care of 4.
Please have a look and let me know whether
the documentation achieves to explain how
connection garbage collection depends on
the proper use of hard and weak references:
http://people.apache.org/~rolandw/client-jdoc/org/apache/http/impl/conn/tsccm/package-summary.html
Ignoring that there is always more cleanup
to be done, steps 1-5 are complete. Step 6
has a low priority. HTTPCLIENT-677 is now
the top item on the todo list for TSCCM.
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: Connection Manager Garbage Collection
Posted by Roland Weber <os...@dubioso.net>.
Hi Sam,
thanks for the warning. Right now, HttpConn is confined
to blocking IO. We'll keep it in mind though, and I'll
also give it some consideration when reviewing the GC.
thanks,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: Connection Manager Garbage Collection
Posted by Sam Berlin <sb...@gmail.com>.
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 <mb...@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
Re: Connection Manager Garbage Collection
Posted by Roland Weber <os...@dubioso.net>.
Hi Mike,
> connection managers should always be shutdown though. I see
> finalizers as an unreliable last resort.
I agree, although I think that shutdown of the JVM is the only
case where it isn't called. I intend to use the finalizers anyway,
because I want to make the cleanup thread optional. Finalizers
will provide a fallback if the cleanup thread is disabled and
the application misbehaves.
>> 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.
Having the obsolete stuff in a single class makes it easier
to sort the rest, and later to identify the references to
the obsolete parts. That way, I don't waste time reformatting
the code I want to delete :-)
> 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.
Right now I'm mostly concerned with making the code better
understandable for others, and easier to handle for myself.
In the long run, I'd like to develop a connection manager with
a more agressive re-use strategy and stricter enforcement of
connections-per-host limits. It's mentioned in the Wiki, in
the last paragraph of the implementation review:
http://wiki.apache.org/jakarta-httpclient/ConnectionManagementDesign#head-620b18cfb586e09ef01a36298be23c41812cf15d
After the restructuring, it should be possible to replace
only the ConnectionPool, while all the other logic related
to garbage collection and operations is re-used. But that's
something for 4.1.
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: Connection Manager Garbage Collection
Posted by Michael Becke <mb...@gmail.com>.
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
Re: Connection Manager Garbage Collection
Posted by Roland Weber <ht...@dubioso.net>.
Hi Mike,
thanks for taking the time to look into this!
> REFERENCE_TO_CONNECTION_SOURCE will only have a reference to a
> ConnectionPool if a connection is checked out and hasn't been GCed (is
> this what you mean by allocated?).
Yes, that's what I meant by allocated.
An application got it, and keeps it.
>> 2. no connection is allocated. Then the TSCCM is GCed,
>> we have a unit test for this. But how are the open
>> connections closed in this scenario? I didn't see a
>> finalizer that would close them.
>
> 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?
> 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.
> 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.
> 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).
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)
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: Connection Manager Garbage Collection
Posted by Michael Becke <mb...@gmail.com>.
Hi Roland,
Below are some comments on your questions.
> 1. a connection is allocated. Then the TSCCM will not be
> GCed, because an entry in REFERENCE_TO_CONNECTION_SOURCE
> prevents it. The entry references the ConnectionPool,
> which is a non-static nested class of the TSCCM, and
> therefore implicitly references the TSCCM.
REFERENCE_TO_CONNECTION_SOURCE will only have a reference to a
ConnectionPool if a connection is checked out and hasn't been GCed (is
this what you mean by allocated?). In the case when a connnection is
checked out and still active there will be many direct references to
the pool and connection manager so this shouldn't be an issue.
In the case when a connection is checked out, lost, and then GCed the
reference from the REFERENCE_TO_CONNECTION_SOURCE will eventually be
removed by the ReferenceQueueThread.
> 2. no connection is allocated. Then the TSCCM is GCed,
> we have a unit test for this. But how are the open
> connections closed in this scenario? I didn't see a
> finalizer that would close them.
Currently there is nothing that closes threads other than shutdown.
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 also attached changes to the LostConnWorker that should
alleviate the issue with the GC test case. Once this change is made I
think you'll be able to 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?
Mike
Re: Connection Manager Garbage Collection
Posted by Roland Weber <os...@dubioso.net>.
Michael Becke wrote:
> Hi Guys,
>
> Think I know the answer off the top of my head, but will dig into the
> code this afternoon to just be sure.
Thanks!
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: Connection Manager Garbage Collection
Posted by Michael Becke <mb...@gmail.com>.
Hi Guys,
Think I know the answer off the top of my head, but will dig into the
code this afternoon to just be sure.
Enjoy,
Mike
On 7/21/07, Roland Weber <os...@dubioso.net> wrote:
> Hi Mike, Oleg, all,
>
> I need some advice on TSCCM garbage collection. I think
> I have the connection GC covered, but there's also the
> GC of the connection manager itself. Based on some
> comments in the code, I believe there are two cases:
>
> 1. a connection is allocated. Then the TSCCM will not be
> GCed, because an entry in REFERENCE_TO_CONNECTION_SOURCE
> prevents it. The entry references the ConnectionPool,
> which is a non-static nested class of the TSCCM, and
> therefore implicitly references the TSCCM.
>
> 2. no connection is allocated. Then the TSCCM is GCed,
> we have a unit test for this. But how are the open
> connections closed in this scenario? I didn't see a
> finalizer that would close them.
>
> I am considering to move the TSCCM into a separate
> package o.a.h.i.conn.tsccm, so I can factor out some
> TSCCM specific classes without cluttering the main
> implementation package. There are way too many nested
> classes to feel comfortable with. This would also
> create a home for some stuff elsewhere which looks
> generic but is really tailored for the TSCCM, such
> as the current selection of parameters.
>
> Oleg, I'm glad you asked me to port the unit tests before
> starting the refactoring. These things are much easier
> to debug one at a time, while I am making the changes.
>
> cheers,
> Roland
>
> ---------------------------------------------------------------------
> 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
Re: Connection Manager Garbage Collection
Posted by Roland Weber <os...@dubioso.net>.
Oleg Kalnichevski wrote:
> There are only two areas in HttpClient 3.x I never touched and hence
> unable to help with: NTLM stuff and MTHCM. If you get no answer from
> Mike, I'll probably have to take a plunge into the MTHCM code
It's mostly a question for confirmation. I don't think you'll
have to dive into the code, I just thought you might happen to
know. Since I don't intend to remove unit tests except for
methods I'll remove, we should be fine in general.
>> Oleg, I'm glad you asked me to port the unit tests before
>> starting the refactoring. These things are much easier
>> to debug one at a time, while I am making the changes.
>>
> Oh well. After all I am not THAT nasty, am I?
No, comrade, you're not evil at all ;-)
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: Connection Manager Garbage Collection
Posted by Oleg Kalnichevski <ol...@apache.org>.
Roland Weber wrote:
> Hi Mike, Oleg, all,
>
> I need some advice on TSCCM garbage collection. I think
> I have the connection GC covered, but there's also the
> GC of the connection manager itself. Based on some
> comments in the code, I believe there are two cases:
>
> 1. a connection is allocated. Then the TSCCM will not be
> GCed, because an entry in REFERENCE_TO_CONNECTION_SOURCE
> prevents it. The entry references the ConnectionPool,
> which is a non-static nested class of the TSCCM, and
> therefore implicitly references the TSCCM.
>
> 2. no connection is allocated. Then the TSCCM is GCed,
> we have a unit test for this. But how are the open
> connections closed in this scenario? I didn't see a
> finalizer that would close them.
>
> I am considering to move the TSCCM into a separate
> package o.a.h.i.conn.tsccm, so I can factor out some
> TSCCM specific classes without cluttering the main
> implementation package. There are way too many nested
> classes to feel comfortable with. This would also
> create a home for some stuff elsewhere which looks
> generic but is really tailored for the TSCCM, such
> as the current selection of parameters.
>
>
Roland,
There are only two areas in HttpClient 3.x I never touched and hence
unable to help with: NTLM stuff and MTHCM. If you get no answer from
Mike, I'll probably have to take a plunge into the MTHCM code
> Oleg, I'm glad you asked me to port the unit tests before
> starting the refactoring. These things are much easier
> to debug one at a time, while I am making the changes.
>
>
Oh well. After all I am not THAT nasty, am I?
Oleg
> cheers,
> Roland
>
> ---------------------------------------------------------------------
> 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