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: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
