[RFC][netx]: unsafe write to HashMap

[Andrew Haley]

On 12/04/11 17:50, 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.
> 
>> 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.

What harm can putIfAbsent possibly do, though?  It's clear and
straightforward.

Andrew.