Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.java.programmer > #15395

Re: Infinite loop from HashMap.keySet iteration.

Show all headers | View raw

On 18.06.2012 22:32, Daniel Pitts wrote:
> On 6/18/12 9:34 AM, Robert Klemme wrote:
>> 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?
> 1. This is legacy code.
> 2. Changing the semantics of this method might introduce bugs. I don't
> feel like debugging this package.
> 3. This is a centrally used library for many different development
> teams. I didn't want to worry about side-effects on other teams.
> 4. This code base *will* be replaced in the next year.
> 5. As an engineer with decades of experience, I have decided that
> changing this code-base was going to be more expensive than leaving it
> alone.

Yep, that sounds reasonable.  I'd just say that iterating via entrySet 
instead of keySet is only marginally semantically different: you'll even 
get more efficient iteration because you spare all the map lookups. 
Plus you are more likely to get the actually corresponding value to a key.

>>> 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?
> getName always returned the same name for the specific factory.
> getResult itself needn't be synchronized.
>
> Again, see the reasons above why I'm not making sweeping changes to this
> package.

Absolutely reasonable.

>>>> 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.
> That is taking care of by the synchronization on the factory.

Not strictly: it will be unsafe if multiple factories can return the 
same (equivalent) name.

> In
> reality, the Factory should have been the key, but that isn't how the
> original coder decided to do things.

Often quoted "historic reasons". :-)  Maybe also the factory should 
really be the value in the map (with the name as key) and cache the key 
internally once it is calculated.

> In any case, the real problem was that there was no happens-before
> between any two "put" method calls, which lead to inconsistent state.

I understood your previous posting that the issue was more with missing 
synchronization between reading (iteration for logging) and writing 
(method addResult()).

Note also that wrapping the Map in a synchronized version does not 
prevent CME.

Kind regards

	robert

-- 
remember.guy do |as, often| as.you_can - without end
http://blog.rubybestpractices.com/

Back to comp.lang.java.programmer | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Infinite loop from HashMap.keySet iteration. Daniel Pitts <newsgroup.nospam@virtualinfinity.net> - 2012-06-14 09:54 -0700
  Re: Infinite loop from HashMap.keySet iteration. markspace <-@.> - 2012-06-14 10:17 -0700
    Re: Infinite loop from HashMap.keySet iteration. Daniel Pitts <newsgroup.nospam@virtualinfinity.net> - 2012-06-14 10:34 -0700
  Re: Infinite loop from HashMap.keySet iteration. Roedy Green <see_website@mindprod.com.invalid> - 2012-06-14 11:34 -0700
    Re: Infinite loop from HashMap.keySet iteration. Daniel Pitts <newsgroup.nospam@virtualinfinity.net> - 2012-06-14 12:19 -0700
  Re: Infinite loop from HashMap.keySet iteration. Robert Klemme <shortcutter@googlemail.com> - 2012-06-18 04:42 -0700
    Re: Infinite loop from HashMap.keySet iteration. Daniel Pitts <newsgroup.nospam@virtualinfinity.net> - 2012-06-18 09:08 -0700
      Re: Infinite loop from HashMap.keySet iteration. Robert Klemme <shortcutter@googlemail.com> - 2012-06-18 18:34 +0200
        Re: Infinite loop from HashMap.keySet iteration. Daniel Pitts <newsgroup.nospam@virtualinfinity.net> - 2012-06-18 13:32 -0700
          Re: Infinite loop from HashMap.keySet iteration. Robert Klemme <shortcutter@googlemail.com> - 2012-06-18 23:26 +0200
            Re: Infinite loop from HashMap.keySet iteration. Daniel Pitts <newsgroup.nospam@virtualinfinity.net> - 2012-06-18 15:30 -0700

csiph-web