Concurrent list modification can cause "java.lang.Illegalargumentexception"?

Question

I have a multi-threaded program that makes concurrent modifications to an unsynchronized list (by bad design decision).

Sometimes, in a seemingly unpredictable way, I get java.lang.IllegalArgumentException: Comparison method violates its general contract! concerning the method compareTo, when invoked a sort on the list.

Reading the documentation and using the test code below with exactly the same methods equals and compareTo, I conclude that the comparison is in accordance with the contract, because I could not reproduce the IllegalArgumentException.

---

After I synced the list (with Collections.synchronizedList) and every part of the code that uses (or modifies) the list, no longer received the IllegalArgumentException (for the time being).

I suppose the cause of the problem was the concurrent modification of the list. If so, why did I receive IllegalArgumentException and not NullPointerException or ConcurrentModificationException?

---

Test code (which does not happen any Exception):

Pai Class:

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.concurrent.atomic.AtomicInteger;

public class Pai implements Comparable<Pai> {
    private static AtomicInteger contador = new AtomicInteger(0);

    private Integer indice;

    protected Integer ordem = 0;

    private String nome;

    private static List <Pai> familia;

    public Pai(String nome) {
        this.indice = contador.incrementAndGet();
        this.nome = nome;
    }

    @Override
    public boolean equals(Object o){
        if (o != null && o instanceof Pai && this != null){
            return (this.indice == ((Pai) o).indice);
        }
        else {
            return false;       
        }
    }

    @Override
    public int compareTo(Pai o){
        if (this != null && o != null)
            return (this.ordem - o.ordem);
        else{
            return 0;
        }
    }

    public String qualNome(){
        return this.nome;
    }

    public static void adicionarFilhos(){
        familia.add(new FilhoTres("3"));
        familia.add(new FilhoDois("2"));
        familia.add(new FilhoUm("1"));
        familia.add(new FilhoTres("3"));
        familia.add(new FilhoUm("1"));
        familia.add(new FilhoUm("1"));
        familia.add(new FilhoDois("2"));
        familia.add(new FilhoTres("3"));
        familia.add(new FilhoUm("1"));
        familia.add(new FilhoDois("2"));
    }

    public static void main(String[] args) {
        familia = new ArrayList<Pai>();

        int i = 0;

        while (i < 1000){
            i++;

            familia.remove(5);

            Collections.sort(familia);

            familia.remove(2);

            Collections.sort(familia);
        }

        System.out.println("fim");
    }

}

Children:

public class FilhoUm  extends Pai {

    public FilhoUm(String nome){
        super(nome);
        this.ordem = 1;
    }

}

public class FilhoDois extends Pai {

    public FilhoDois(String nome){
        super(nome);
        this.ordem = 2;
    }
}

public class FilhoTres extends Pai {

    public FilhoTres(String nome){
        super(nome);
        this.ordem = 3;
    }

}

Answer 1

Your methods compareTo, equals and hashCode are violating their general contracts yes. Let’s look at them starting with compareTo:

Method compareTo

@Override
public int compareTo(Pai o){
    if (this != null && o != null)
        return (this.ordem - o.ordem);
    else{
        return 0;
    }
}

The expression this != null is always true, because there is no possible way for the object this be it null. So it boils down to this:

@Override
public int compareTo(Pai o) {
    if (o != null) return this.ordem - o.ordem;
    return 0;
}

Note the return 0. It is only possible if the value of the o for null. In the meantime, this is what interface javadoc Comparable:

Note that null is not an instance of any class, and e.compareTo(null) should throw a NullPointerException

What translating into Portuguese is:

Note that null is not an instance of any class, and that e.compareTo(null) should launch a NullPointerException

I mean, you can simplify your compareTo to look like this:

@Override
public int compareTo(Pai o) {
    return this.ordem - o.ordem;
}

Method equals

Now let’s see the equals:

@Override
public boolean equals(Object o){
    if (o != null && o instanceof Pai && this != null){
        return (this.indice == ((Pai) o).indice);
    }
    else {
        return false;       
    }
}

The understatement this != null is always true, and therefore can be eliminated (there is no way this was null). Moreover, if o != null is fake, so o instanceof Pai will also be false, and therefore the o != null is unnecessary and redundant. Therefore, your method equals comes down to this:

@Override
public boolean equals(Object o) {
    if (o instanceof Pai) {
        return this.indice == ((Pai) o).indice;
    } else {
        return false;
    }
}

Which in turn can be simplified in this:

@Override
public boolean equals(Object o) {
    return o instanceof Pai && this.indice == ((Pai) o).indice;
}

However, I notice that the field indice is the type Integer, and not int. Compare two objects of the type Integer using the operator == is as problematic as comparing Strings using the ==, and can result in unpleasant surprises. For example:

Integer a = new Integer(777);
Integer b = new Integer(777);
System.out.println(a == b);    // Escreve "false".

So, that makes yours equals:

@Override
public boolean equals(Object o) {
    return o instanceof Pai && Objects.equals(this.indice, ((Pai) o).indice);
}

There is one more but... The class Pai has subclasses. And it’s never a good idea to have objects from different classes that can be equal. Like this method equals will be inherited by all subclasses, we will have it possible that two objects of different classes (but both subclasses of Pai) would be considered equal by having the same values in the field indice. Therefore, this would be a better implementation for your equals:

@Override
public boolean equals(Object o) {
    return o != null
            && o.getClass() == this.getClass()
            && Objects.equals(this.indice, ((Pai) o).indice);
}

Method hashCode

And now your method hashCode. Well... You did not implement the hashCode! It happens that it is fundamental, because the equals as described in javadoc of the method equals(Object):

Note that it is generally necessary to override the hashCode method Whenever this method is overridden, so as to maintain the general Contract for the hashCode method, which States that Equal Objects must have Equal hash codes.

What translating into Portuguese is:

Note that it is usually necessary to overwrite the method hashCode where this method is superscripted in order to maintain the general contract for the method hashCode, which specifies that equal objects should have equal hash codes.

And looking at the javadoc of the method hashCode():

If two Objects are Equal According to the equals(Object) method, then Calling the hashCode method on each of the two Objects must Produce the same integer result.

What translating into Portuguese is:

If two objects are equal according to the method equals(Object), then call the method hashCode in each of these two objects must produce the same entire result.

A very simple implementation for your method hashCode that works in conjunction with the equals and with the compareTo that would be it:

@Override
public int hashCode() {
    return indice == null ? 0 : indice;
}

compareTo consistent with equals

Finally, it is expected that a and b are two equal elements (ie a.equals(b) is true), so it is expected that a.compareTo(b) is zero. However, this does not occur in your code. Although the Comparator Allow this, he strongly recommends against:

The natural Ordering for a class C is said to be consistent with equals if and only if e1.compareTo(e2) == 0 has the same Boolean value as e1.equals(e2) for Every e1 and e2 of class C. [...] It is strongly Recommended (though not required) that natural orderings be consistent with equals.

What translating into Portuguese:

The natural ordination of a class C is said to be consistent with the equals if and only if e1.compareTo(e2) == 0 has the same Boolean value as e1.equals(e2) for each e1 and e2 class C. [...] It is strongly recommended (though not required) that natural ordinations be consistent with the equals.

Once you order using the field ordem and checks equality using the field indice, then its ordering is not consistent with the equals.

I note that in your code, you never seem to set a value for the field indice, and so I think what you really wanted was to check equality through the field ordem. If so, then you would erase the field indice and these would be his equals and its hashCode:

@Override
public boolean equals(Object o) {
    return o != null
            && o.getClass() == this.getClass()
            && Objects.equals(this.ordem, ((Pai) o).ordem);
}

@Override
public int hashCode() {
    return ordem == null ? 0 : ordem;
}

If you really want to use the indice for some purpose other than ordering, so I recommend that you define that two objects are equal when the fields indice and ordem are equal, and therefore their equals and hashCode would look like this:

@Override
public boolean equals(Object o) {
    return o != null
            && o.getClass() == this.getClass()
            && Objects.equals(this.indice, ((Pai) o).indice)
            && Objects.equals(this.ordem, ((Pai) o).ordem);
}

@Override
public int hashCode() {
    return Objects.hash(indice, ordem);
}

There’s one more problem, your compareTo will give a NullPointerException unwanted when the field ordem of any of the objects is null.

However, you may prefer to put the fields ordem and indice as int, and in this case, although you can replace the use of Objects.equals of his method equals back to the ==, it will continue to work if you use Objects.equals.

With all this, you’ll leave yours compareTo consistent with the equals and will not break the contract of any of the methods compareTo, equals or hashCode.