hg: lambda/lambda/jdk: bug fixes and unit test for Map Defaults

[Joe Bowbeer]

putIfAbsent was not designed to be compatible with null values. It was
designed for the null-incompatible ConcurrentMap interface. I don't think a
null-compatible implementation of CM is possible, BTW.

So the prospects for putIfAbsent in Map are limited from the start. In your
analysis you assume that the user cares about the return value (which is
where the problem arises) but might this not be the case?

Joe
On Mar 27, 2013 1:25 AM, "Peter Levart" <peter.levart at gmail.com> wrote:

>  On 03/27/2013 07:40 AM, Joe Bowbeer wrote:
>
> I was not expecting Map to define a default impl. for what was originally
> a ConcurrentMap method, btw.
>
> If it does, however, and if Map supports null - as all the base
> Collections do - then shouldn't all the default method implementations
> support null as well? (Support is the correct term here, as the Collections
> spec makes no mention of tolerate.)
>
>
> The new definition still supports null, but differently.
>
>  In Map there is a distinction between !contains(key) and get(key)==null.
> I think putIfAbsent is even defined in terms of contains() and not get().
>
>
> It is defined, yes, but the part about null values "wasn't exploited in
> reality", since all ConcurrentMap implementations in JDK don't support null
> values and I haven't yet seen one that does. Have you? The debate is about
> whether it is appropriate to *change* the definition of putIfAbsent. I
> think it is. Because now, that putIfAbsent is being defined for plain Maps
> which we know do support null values, the question about nulls becomes
> relevant. Which is the most appropriate definition of putIfAbsent for Maps
> that do support null values (the only constraint being that method
> signature should not change)? Current definition (as defined until now) is
> dubious, since when putIfAbsent returns null, the user does not know
> whether null has already been mapped and was therefore not replaced with
> new value or there was no mapping yet and new value has been entered into
> the Map. This non-determinism is not making such putIfAbsent usable at all.
> With new definition, the null return is more deterministic. You still don't
> know whether there was already a mapping to null present in the map or
> there was no mapping, but you are certain that new value landed in the Map.
> I think this is more appropriate. In either way, if one wants to use
> putIfAbsent, he should not put null values in the Map.
>
> Regards, Peter
>
>  How can we justify not fully supporting null in some Map methods?
>
> I can't come up with a rationale or a loophole to exploit.
> On Mar 27, 2013 12:01 AM, "Mike Duigou" <mike.duigou at oracle.com> wrote:
>
>> I've pushed another update that has putIfAbsent treat null values as
>> absent. This allows putIfAbsent to be restored in a couple of the defaults.
>> This feels like the right thing to do though some null-lover will
>> eventually be unhappy that his null value got trampled. The remove() and
>> replace() methods still distinguish between absent keys and present keys
>> with null values. I think this is appropriate.
>>
>>  The alternative to treating null values as absent is to add additional
>> code to distinguish between absent and nulls values everywhere. I attempted
>> this in
>>
>>  http://hg.openjdk.java.net/lambda/lambda/jdk/rev/8561d74a9e8f
>>
>>  It wasn't much more successful because the value mappers use null for
>> signalling.
>>
>>  The current specification and behaviour seems about as good as can be
>> achieved while still tolerating nulls and without introducing Optional.
>>
>>  Mike
>>
>>  On Mar 25 2013, at 16:41 , Mike Duigou wrote:
>>
>>  My intention with the first cut of the unit test was to avoid changing
>> the method specification while having consistent behaviour across all of
>> the Map implementations. It's certainly possible I made mistakes and
>> changed the contract of the defaults in ways I didn't intend.
>>
>>  I think perhaps that changing putIfAbsent to allow replacement of null
>> values would help a lot to making the operations more consistent. I am also
>> going to look at Sam's suggestion of using replace() rather than put() in a
>> few places. Using replace() almost ensures though that we'll introduce more
>> retry loop implementations. This may be fine though.
>>
>>  Mike
>>
>>
>>  On Mar 25 2013, at 14:57 , Peter Levart wrote:
>>
>>  Hi Mike,
>>
>> I see default Map.computeIfAbsent has been chosen to not be atomic and
>> rather support null values more naturally. That's ok if JDK ConcurrentMap
>> implementations provide atomic overrides. Other 3rd party implementations
>> will follow soon.
>>
>>  But I have doubts about default Map.compute(). On one hand it contains
>> the usual disclaimer:
>>
>>       900      * <p>The default implementation makes no guarantees about
>>       901      * synchronization or atomicity properties of this method
>> or the
>>       902      * application of the remapping function. Any class
>> overriding
>>       903      * this method must specify its concurrency properties.  In
>>       904      * particular, all implementations of subinterface {@link
>>       905      * java.util.concurrent.ConcurrentMap} must document
>> whether the
>>       906      * function is applied exactly once atomically. Any class
>> that
>>       907      * permits null values must document whether and how this
>> method
>>       908      * distinguishes absence from null mappings.
>>
>> ...but on the other hand it tries to be smart:
>>
>>       924      * In concurrent contexts, the default implementation may
>> retry
>>       925      * these steps when multiple threads attempt updates.
>>
>> Now this last sentence may indicate that a ConcurrentMap implementation
>> that does not override compute() might safely be used with default
>> Map.compute() and be atomic. It was atomic until putIfAbsent() was replaced
>> with plain put() to correct the "existing null value" behavior. The
>> retry-loop is only needed when the optimistic operation is not successful.
>> But put() is not optimistic. It always "succeeds". And retrying after the
>> put() only makes things worse: "Oh, somebody was quicker than me and I have
>> just overwritten his value - never mind, I'll try to make some more damage
>> in next loop..."
>>
>> If the damage was done already, then there's no point in repeating the
>> loop. Further, what's the point in using optimistic operations:
>> replace(key, oldValue, newValue) and remove(key, oldValue) with retry-loop
>> on one hand and then just stomping over with put() on the other hand. If
>> the default Map.compute() method is declared non-atomic, then plain
>> put(key, newValue) instead of replace(key, oldValue, newValue) and
>> remove(key) instead of remove(key, oldValue) could be used and no
>> retry-loop...
>>
>> The same goes for default Map.merge().
>>
>> Regards, Peter
>>
>> P.S. What do you think about changing the specification of putIfAbsent to
>> always overwrite null values? It could make all these things simpler and
>> more consistent. And it would not break anything.
>>
>>
>> On 03/25/2013 08:21 PM, mike.duigou at oracle.com wrote:
>>
>> Changeset: c8d40b7e6de3
>> Author:    mduigou
>> Date:      2013-03-20 20:32 -0700
>> URL:       http://hg.openjdk.java.net/lambda/lambda/jdk/rev/c8d40b7e6de3
>>
>> bug fixes and unit test for Map Defaults
>>
>> ! src/share/classes/java/util/HashMap.java
>> ! src/share/classes/java/util/Map.java
>> + test/java/util/Map/Defaults.java
>>
>>
>>
>>
>>
>>
>>
>