Path: csiph.com!usenet.pasdenom.info!news.albasani.net!fu-berlin.de!uni-berlin.de!individual.net!not-for-mail
From: Robert Klemme <shortcutter@googlemail.com>
Newsgroups: comp.lang.java.programmer
Subject: Re: Infinite loop from HashMap.keySet iteration.
Date: Mon, 18 Jun 2012 18:34:10 +0200
Lines: 44
Message-ID: <a493k2FpcfU1@mid.individual.net>
References: <EHoCr.13285$ji7.6276@newsfe20.iad> <313a279f-0e72-4431-9879-c0deb4385f1c@googlegroups.com> <RnIDr.19181$l_1.6246@newsfe21.iad>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
X-Trace: individual.net BdHUA0ldcpi3M2o9eiAIWg65pFgaMgBEFKDmFoXPXSw8vJ87j8DsE396ODhAxDpDY=
Cancel-Lock: sha1:W8R0JD8Lrtffu6DCHMpi92dnFPc=
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20120430 Thunderbird/12.0.1
In-Reply-To: <RnIDr.19181$l_1.6246@newsfe21.iad>
Xref: csiph.com comp.lang.java.programmer:15381

On 06/18/2012 06:08 PM, Daniel Pitts wrote:
> On 6/18/12 4:42 AM, Robert Klemme wrote:

>> [refactoring from keySet() to entrySet()]

> Agreed. However, the code I was handed used keySet(). I didn't feel like
> refactoring this (for many reasons).

For example?

> Nope, that is not, in fact, the original author's intent. There was no
> other synchronizing on the resultMap. The original author was only
> worried about protecting the key, not the whole map. This of course is
> not sufficient, but it is what they implemented.

The key is a String which does not need synchronization because it is 
immutable.  Did you mean to say "factory" instead?  I have no idea what 
ResultFactory actually does but getName() looks like something which 
always returns the same name.  If that was the case then no 
synchronization would be necessary.  And if getResult() involves a 
calculation which needs to be thread safe then that method could be 
synchronized.  Can you share more details about that class?

>> Result result = resultsMap.get(factory.getName());
>> if (result == null) {
>> result = new Result();
>> resultsMap.put(factory.getName(), result);
>> }
>> result.recordResult(factory.getResult());
>> }
>> }
>>
>> Or the synchronize(resultsMap) was simply forgotten.
> Exactly, which is why I ensure resultsMap is a synchronized Map instead.

This won't make the code entirely thread safe - at least if you want to 
ensure that the fist thread which detects the missing entry also adds it 
to the Map and - maybe more important - that getResult() is invoked only 
once per key.  Collections.synchronizedMap() only ensures all accesses 
are synchronized - not more.

Kind regards

	robert