[RFC][netx]: unsafe write to HashMap

[Denis Lila]

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.

> 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.

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
...