All, as I said, this was stuff that I just tried and have been using without any
serious evaluation. I have not seen problems out of this code and I
specifically did note the unlocked condition on the loaderTable usage. When I
look at this code now, I see opportunity for put's to be missed because of
unlocked changes in hash chains from other threads. This could cause ODD class
cast exceptions in applications expecting to share class loaders I guess. But
also, if not, it would just cause a miss on the "get" and cause a second
download of the same codebase. There are probably several other things that
might go really wrong.
I'm glad Chris looked hard at this. As I suggested in the code posted into the
issue, it was not production changes, just stuff that I had played around with.
Yes, I do use it in production, but remember that because of my changes for
deferred unmarshalling elsewhere, a user practically would only ever initiate
downloading of a single codebase through a single thread, so that's the most
likely reason why I haven't noted "bad" behavior.
I see that he's formulated a different change set. It looks like putting
synchronized(loaderTable) around the three accesses to the loaderTable would
keep things sane for multiple threads accessing separate class loaders. But, it
may be that I did that, and found some other problem. I did this quite some
time ago and don't have notes on what I discovered otherwise.
Gregg
On 4/13/2011 11:56 AM, Christopher Dolan wrote:
Gregg,
Thanks again for the insight on PreferredClassProvider. I'm reviewing
your patch from RIVER-336 and I think I see a serious issue. The
loaderTable field is an unsynchronized HashMap, yet you access it from
multiple synchronization contexts. The lookupLoader method with your
patch attached looks like this, in part. I put "-->" markers on lines
where the loaderTable is used.
synchronized (loaderTable) {
/*
* Take this opportunity to remove from the table entries
* whose weak references have been cleared.
*/
Object ref;
while ((ref = refQueue.poll()) != null) {
if (ref instanceof LoaderKey) {
LoaderKey key = (LoaderKey) ref;
if( key.isActive() )
--> loaderTable.remove(key);
} else if (ref instanceof LoaderEntry) {
LoaderEntry entry = (LoaderEntry) ref;
if (!entry.removed) { // ignore entries
removed below
--> loaderTable.remove(entry.key);
}
}
}
}
/*
* Look up the codebase URL path and parent class loader pair
* in the table of RMI class loaders.
*/
LoaderKey key = getKey( urls, parent );
synchronized( key ) {
--> LoaderEntry entry = (LoaderEntry) loaderTable.get(key);
ClassLoader loader;
if (entry == null ||
(loader = (ClassLoader) entry.get()) == null)
{ entry = new LoaderEntry(key, loader);
.... snip ....
--> loaderTable.put(key, entry);
}
return loader;
}
Putting the loaderTable access in the "synchronized (key)" block means
that the HashMap can be modified from multiple threads. It seems to me
that you must either change the loaderTable to be some other kind of
concurrent map, or redo the locking so the put() is synchronized on
loaderTable.
Furthermore, I don't see the point of the isActive()/setActive()
methods. The active field is always set to true and never to false, so
why is this checked? Is this vestigial code?
Chris
-----Original Message-----
From: Christopher Dolan [mailto:[email protected]]
Sent: Wednesday, April 13, 2011 9:29 AM
To: [email protected]
Subject: RE: SocketPermission and LookupLocatorDiscovery vs. Reggie
scalability
For web searchers from the future, I found the patch attached to this
defect:
https://issues.apache.org/jira/browse/RIVER-336
See Gregg's 02/Apr/10 comment on that issue. Thanks, Peter and Gregg.
Chris
