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 Thu, 14 Apr 2011 21:40:10 GMT
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:gregg@wonderly.org] 
Sent: Thursday, April 14, 2011 3:55 PM
To: dev@river.apache.org
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: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
>


Mime
View raw message