[RFC][netx]: unsafe write to HashMap

[Dr Andrew John Hughes]

On 12:50 Tue 12 Apr     , Denis Lila wrote:
> Hi Andrew.
> 
> I'm sorry it took so long to answer this. I've been busy
> with a few other things.
> 
> > No, this isn't enough. This piece of code:
> > 
> > private URL findBestUrl(Resource resource) {
> > 1. DownloadOptions options = downloadOptions.get(resource);
> > 2. if (options == null) {
> > 3. options = new DownloadOptions(false, false);
> > }
> > 
> > may be subject to a race from addResource. Line 1 may retrieve null,
> > but that thread may then be interrupted before line 2 can be executed.
> > In the meantime, the resource could be added to downloadOptions and
> > this thread would still have the old stale value.
> 
> I don't think this is a problem findBestUrl never races with addResource.
> It's only called from initializeResource, which isn't called unless
> the resource has been already added.
> 

That may be true now, but future code could break this implicit assumption
without realising it.  It also wasn't that obvious to me as the code stands now.

> > It's also odd code in that this new instance is created but not stored
> > in the map. So it seems possible that many threads could call this
> > method, all get a return value of null and create new instances of
> > DownloadOptions. The control flow in this class is not very clear,
> > but I presume the null check means that there is a possibility that
> > this method can be called before the resource is added in addResource.
> > 
> > I would suggest replacing those three lines with:
> > 
> > DownloadOptions options = downloadOptions.putIfAbsent(resource, new
> > DownloadOptions(false,false));
> 
> I agree that it's odd, but this code has deadlocks in it and it's far
> more complex than necessary so a lot of it will be re-written. I would
> like to wait until that happens before pushing something like your
> suggestion, because it is likely to become unecessary.
> 

Hmmm, you're the one who wanted to change it in the first place...

> Regards,
> Denis.
> 
> ----- Original Message -----
> > On 12:13 Tue 05 Apr , Denis Lila wrote:
> > > Hi.
> > >
> > > I think I found a concurrency problem in
> > > ResourceTracker.addResource. This method is not static.
> > > It is called from different threads, and it writes to
> > > downloadOptions which is a static HashMap. This is not
> > > safe. The patch uses a ConcurrentHashMap. That should
> > > be enough to fix it.
> > >
> > > Ok to push?
> > >
> > > Regards,
> > > Denis.
> > 
> > 
> 
> > 
> > This ensures, atomically, that any existing value is retrieved and
> > stored
> > in options, but if one is not present, the default is added and
> > returned.
> > With addResource using put instead of putIfAbsent, it should still
> > override
> > a default added by this method.
> > 
> > This also means there's a reason to follow Andrew's suggestion, as
> > putIfAbsent is unique to ConcurrentHashMap. In general, the interface
> > type (Map) should be used rather than the specific type to allow easy
> > transition between different implementations. It prevents code using
> > implementation-specific methods which would make later implementation
> > changes harder e.g. if this class had been using HashMap directly, it
> > may have relied on HashMap-only methods.
> > 
> > > diff -r 62011060e7ea ChangeLog
> > > --- a/ChangeLog Tue Apr 05 12:07:10 2011 -0400
> > > +++ b/ChangeLog Tue Apr 05 12:12:33 2011 -0400
> ...

-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37