river-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Dolan" <christopher.do...@avid.com>
Subject RE: SocketPermission and LookupLocatorDiscovery vs. Reggie scalability
Date Wed, 13 Apr 2011 16:56:35 GMT

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

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?


-----Original Message-----
From: Christopher Dolan [mailto:christopher.dolan@avid.com] 
Sent: Wednesday, April 13, 2011 9:29 AM
To: dev@river.apache.org
Subject: RE: SocketPermission and LookupLocatorDiscovery vs. Reggie

For web searchers from the future, I found the patch attached to this
See Gregg's 02/Apr/10 comment on that issue. Thanks, Peter and Gregg.

View raw message