[concurrency-interest] CopyOnWriteArrayNavigableSet review followup

Mike Duigou openjdk at duigou.org
Wed Dec 30 15:18:49 EST 2015


Dr Heinz M. Kabutz <heinz at javaspecialists.eu> wrote:
> At the moment you can construct a CopyOnWriteArrayNavigableSet for a 
> type that does not have a natural ordering and without specifying a 
> comparator.  I think that makes our lives a lot harder in the 
> implementation.  Better would be to force the user to pass in a correct 
> Comparator for that type.  That way we know that we always have one and 
> we don't need to check for null
anymore.  ... It is easy enough to pass in Comparator.naturalOrder() and 
that also checks that E is in fact Comparable.

This seems like a very reasonable idea. I have always disliked the use 
of null as a sentinel value.

I have also added the static create() factory methods suggested by Chris 
Povirk though I did not make the constructors private as this would be 
too unusual for a JDK class.

> Dr Heinz M. Kabutz <heinz at javaspecialists.eu> wrote:

> 1. In the JavaDoc comment, probably use <Handler> instead of 
> <Handler>

The entire block is wrapped with {@code } so this would result in < 
being rendered rather than <

> 2. I would probably delegate to the Integer.compare(int, int) method in 
>  your Handler otherwise we might get overflows resulting in incorrect 
> comparisons.

Good catch. Done.

> 3. Inside class X in the JavaDocs, use the diamond operator

Done.

> 4. Unless there is a compelling reason, I would make both comparator 
> and
al fields private.

The package private access is done so that accessor methods are not 
needed for BoundedNavigableSet. This is typical within the 
j.u.concurrent classes.

> 5. I would make the internal constructor also private:

It is package private so that it can be called from tests.

6. Where you use the wrapped COWArrayList for locking, instead of
synchronized(super.al.lock)

You may be looking at an old implementation of COWArrayList. In the 
latest JDK9 repo the type of locking has been changed to a standard Java 
monitor.

> 7. I dislike turning off unchecked warnings.

Agreed. I tried your suggestion and found that warnings were still 
generated for the comparator assignment so I opted to leave things as 
they are/were.

> 8. Since we have type erasure in generics, it would probably be also a  
> bit more accurate to use Object[] instead of E[].  See the other 
> classes in java.util.* for comparison.  I understand why you did it - 
> to get Arrays.binarySort() to work.  However, it is a bit messy that 
> one could construct a CopyOnWriteArrayNavigableSet with a generic type 
> that is not Comparable and without a Comparator and where we would get 
> an ClassCastException the moment we use it.

Agreed that the use of Object[] or E[] is inconsistent across 
java.util.* Generally the policy has been to use whichever is 
convenient.

> 9. The fromLoc and toLoc methods both have the same code for finding 
> the
comparator

Removed by eliminating null for natural order.

> 10. I am concerned by the number of methods that are being called 
> whilst holding locks.  Whilst I don't have any concrete example, I am 
> concerned that this could lead to deadlocks.

We only lock on one private object from our own instance so I don't 
believe there's any risk of deadlock.


Thank you for the feedback and to those who provided earlier feedback 
and off-list feedback.

Once I have done some tidying and retesting I will publish an updated 
version of the source.

Mike


More information about the Concurrency-interest mailing list