Thanks for the accumulated thoughts, Gregg. It's hard to pick up details like that from old mail threads and Jira comments, so it's helpful to me that you reiterated those points.
In that light, I hope that my proposed change in RIVER-396 will be considered for inclusion in the trunk. I'm starting smoke testing on that patch today. Chris -----Original Message----- From: Gregg Wonderly [mailto:[email protected]] Sent: Thursday, April 14, 2011 3:55 PM To: [email protected] Cc: Christopher Dolan Subject: Re: SocketPermission and LookupLocatorDiscovery vs. Reggie scalability 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 >
