Issue with ConcurrentSkipListSet remove()

Question

I'm not sure if there is an issue with java.util.concurrent.ConcurrentSkipListSet? I am trying to add some objects to ConcurrentSkipListSet (ordering maintained by my own comparator). After adding I mutate the state of some of the objects. The properties that I change include the ones that are used in the comparator. Now when I try to remove some objects, it fails. The object doesn't get removed from the ConcurrentSkipListSet and the remove(Object) returns a false.

If I replace ConcurrentSkipListSet with a TreeSet I do not see this behaviour.

Not sure if I'm doing really something stupid here or missing something :(. Here is a sample code snippet.

public class TreeVsSkip {
static TreeSet ts = new TreeSet(new Comparator(){

    @Override
    public int compare(Object o1, Object o2) {
        if(((Emp)o1).empid == ((Emp)o2).empid){
            return 0;
        }
        if(((Emp)o1).empid > ((Emp)o2).empid){
            return 1;
        }
        return -1;
    }

});

static ConcurrentSkipListSet<Emp> csls = new ConcurrentSkipListSet(new Comparator(){

    @Override
    public int compare(Object o1, Object o2) {
        if(((Emp)o1).empid == ((Emp)o2).empid){
            return 0;
        }
        if(((Emp)o1).empid > ((Emp)o2).empid){
            return 1;
        }
        return -1;
    }

});
public static void main(String ...strings ){
    System.out.println("Testing Tree...");
    Emp e1 = new Emp(1,"abc");

    ts.add(e1);
    ts.add(new Emp(2,"pqr"));
    ts.add(new Emp(3,"xyz"));
    System.out.println(ts);
    e1.setName("test");
    e1.setId(8);
    System.out.println(ts);
    ts.remove(new Emp(3,"xyz"));
    System.out.println(ts);


    System.out.println("Testing ConcurrentSkipSet...");
    e1.setName("abc");
    e1.setId(1);
    csls.add(e1);
    csls.add(new Emp(2,"pqr"));
    csls.add(new Emp(3,"xyz"));
    System.out.println(csls);
    e1.setName("test");
    e1.setId(8);
    System.out.println(csls);
    System.out.println(csls.remove(new Emp(3,"xyz")));
    System.out.println(csls);
}

static class Emp {
    int empid;
    String name;
    Emp(int id, String n){
        empid = id;
        name = n;
    }
    void setName(String pname){
        name = pname;
    }
    void setId(int pID){
        empid = pID;
    }
    public String toString(){
        return "EmpId:"+empid+"Name:"+name;
    }
}

}

The output looks like:

Testing Tree...
[EmpId:1Name:abc, EmpId:2Name:pqr, EmpId:3Name:xyz]
[EmpId:8Name:test, EmpId:2Name:pqr, EmpId:3Name:xyz]
[EmpId:8Name:test, EmpId:2Name:pqr]
Testing ConcurrentSkipSet...
[EmpId:1Name:abc, EmpId:2Name:pqr, EmpId:3Name:xyz]
[EmpId:8Name:test, EmpId:2Name:pqr, EmpId:3Name:xyz]
false
[EmpId:8Name:test, EmpId:2Name:pqr, EmpId:3Name:xyz]

Note that this behaviour is not consistent. Sometimes the element gets removed.

I use java version "1.8.0_131" on OS X ver 10.11.6.

Apologies for shabby code. Cooked it up in a hurry.

Thanks.

Answer 1

When you add an item to a set or use as a key in a map, you can't change any field which is used for comparison e.g. compareTo or Comparator or hashCode/equals as appropriate or you have corrupted the collection.

The only way to remove such items is to iterate over all the elements and remove it via the Iterator.

BTW I would avoid using a static collection but if you must you can do

static final Set<Emp> csls = new ConcurrentSkipListSet(
                                            Comparator.comparing(e -> e.empid));

If you are going to change the empid you would need to remove it first, change it and then add it back in. To avoid doing this by accedient I would make the field final setting it only in the constructor.