Thanks for the feedback. On Jun 19 2013, at 21:51 , David Holmes wrote:
> Hi Mike, > > On 20/06/2013 10:22 AM, Mike Duigou wrote: >> Hello all; >> >> This is a fix to the Map.compute() default method and HashMap.compute() >> implementation to correct the handling of keys mapped to null values when >> the function returns null. This situation should result in the key being >> removed but it was not. >> >> http://cr.openjdk.java.net/~mduigou/JDK-8017088/0/webrev/ > > In Map if we get to line 1030 we retry the loop, but we don't re-read the > currently mapped value even though the comment indicates it has changed. I > don't quite grok this. The putIfAbsent is assigning to oldValue the current value. If it turns out to be null (meaning the value was previously absent) then we return. If it is non-null then our put failed so we loop. We don't have to read it again. (yeah, I'm not a big fan of assignment in conditionals [hence my frequent use of yoda tests] but the existing default impls use them heavily). > Also watch for spacing: if(oldValue -> if (oldValue Oops, forgot to restyle. > >> I am strongly considering moving all of the current looping defaults in Map >> to ConcurrentMap and replacing them with implementations which throw >> ConcurrentModificationException rather than retrying. The current >> implementation of Map.compute() default comes close to breaking atomicity by >> adding the containsKey() check. (It doesn't because the subsequent remove() >> when oldValue = null can't succeed for any known ConcurrentMap >> implementations). > > I seem to recall this being discussed before but can't find relevant emails > ... I brought it up to EG as one of the topics in this message: http://mail.openjdk.java.net/pipermail/lambda-libs-spec-experts/2013-April/001645.html Nobody expressed an opinion on the idea and I got distracted and haven't had a chance to follow up. > You would need to revise specs regarding throwing, or not, of CME. Yes, understood. Mike
