Mailing-List: contact dev-help@river.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@river.apache.org
Received-SPF: pass (nike.apache.org: domain of
 SRS0=Z8FPoU=XG=wonderly.org=gregg@yourhostingaccount.com designates
 65.254.253.86 as permitted sender)
Message-ID: <4DA75F1C.20505@wonderly.org>
Date: Thu, 14 Apr 2011 15:54:52 -0500
From: Gregg Wonderly <gregg@wonderly.org>
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US;
 rv:1.9.2.15) Gecko/20110303 Thunderbird/3.1.9
MIME-Version: 1.0
To: dev@river.apache.org
CC: Christopher Dolan <christopher.dolan@avid.com>
Subject: Re: SocketPermission and LookupLocatorDiscovery vs. Reggie
 scalability
References: 
 <77F1E32F67C8D5479858C0C7E93EB465063E4FC8@WAL-MAIL.global.avidww.com>
 <BD439E2F-FA60-4C43-964A-FC71D3ADE252@wonderly.org>
 <77F1E32F67C8D5479858C0C7E93EB465064615E9@WAL-MAIL.global.avidww.com>
 <4DA5401B.8010908@zeus.net.au>
 <77F1E32F67C8D5479858C0C7E93EB465064CC2E2@WAL-MAIL.global.avidww.com>
 <77F1E32F67C8D5479858C0C7E93EB465064CC59C@WAL-MAIL.global.avidww.com>
In-Reply-To: 
 <77F1E32F67C8D5479858C0C7E93EB465064CC59C@WAL-MAIL.global.avidww.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Sender: Gregg Wonderly <gregg@wonderly.org>

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:christopher.dolan@avid.com]
> Sent: Wednesday, April 13, 2011 9:29 AM
> To: dev@river.apache.org
> 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
>