When catch doesn't actually catch anything

I had a program crash because of bad data stored in a database recently. This confused me, because I thought I had a catch to prevent this.

The intent of the following code is to compare employee badge numbers and sort them. If there's an error, return -1 and soldier on -- don't stop because one of several thousand badge numbers is wrong:

public int compare(Employee t, Employee t1) {
    Integer returnValue = -1;
    try {
        Integer tb = Integer.parseInt(t.getBadgeNumber());
        Integer t1b = Integer.parseInt(t1.getBadgeNumber());
        returnValue = tb.compareTo(t1b);
    } catch (Exception e) {
        returnValue = -1;//useless statement, I know.
    }
    return returnValue;
}

When the bad badge number hit (as t in this case), I got an "java.lang.IllegalArgumentException: Comparison method violates its general contract!" error instead of returning the -1 in the catch.

What don't I understand about the catch here?

The full stacktrace:

16-May-2018 14:28:53.496 SEVERE [http-nio-8084-exec-601] org.apache.catalina.core.StandardWrapperValve.invoke Servlet.service() for servlet [RequestServlet] in context with path [/AppearanceRequest] threw exception
 java.lang.IllegalArgumentException: Comparison method violates its general contract!
at java.util.TimSort.mergeHi(TimSort.java:868)
at java.util.TimSort.mergeAt(TimSort.java:485)
at java.util.TimSort.mergeForceCollapse(TimSort.java:426)
at java.util.TimSort.sort(TimSort.java:223)
at java.util.TimSort.sort(TimSort.java:173)
at java.util.Arrays.sort(Arrays.java:659)
at java.util.Collections.sort(Collections.java:217)
at org.bcso.com.appearancerequest.html.NotifierHTML.getHTML(NotifierHTML.java:363)
at org.bcso.com.appearancerequest.AppearanceRequestServlet.processRequest(AppearanceRequestServlet.java:96)
at org.bcso.com.appearancerequest.AppearanceRequestServlet.doGet(AppearanceRequestServlet.java:565)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:618)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:725)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:301)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
at org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:52)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:239)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
at org.netbeans.modules.web.monitor.server.MonitorFilter.doFilter(MonitorFilter.java:393)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:239)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)
at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:219)
at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:106)
at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:503)
at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:136)
at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:74)
at org.apache.catalina.valves.AbstractAccessLogValve.invoke(AbstractAccessLogValve.java:610)
at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:88)
at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:516)
at org.apache.coyote.http11.AbstractHttp11Processor.process(AbstractHttp11Processor.java:1015)
at org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:652)
at org.apache.coyote.http11.Http11NioProtocol$Http11ConnectionHandler.process(Http11NioProtocol.java:222)
at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1575)
at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.run(NioEndpoint.java:1533)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
at java.lang.Thread.run(Thread.java:745)

The calling code:

    List<Employee> employeeList = DatabaseUtil.getEmployees();
    Collections.sort(employeeList, new BadgeComparator());

The exception (whatever it was) was caught by catch (Exception e). You didn't log this exception, so you don't know what it was. You should log it somehow so you know what really happened.

The problem occurs when you return -1. This allows for the possibility of inconsistent ordering, which Java's current sorting algorithm sometimes catches. In short, returning -1 on an error means that you are asserting that both a < b and b < a are true, because the exception will be caught in both cases. This is logically incorrect. The sorting algorithm detects this and throws the IllegalArgumentException. Note that the compare method is not in your stack trace; it's the call to Collections.sort.

In addition to logging the exception, handle it before you even get to the comparison step in your program. If you have to parse the string as an integer, do that when creating the Employee objects, so that the validation occurs before you even get to the sorting step in your program. A Comparator shouldn't have to validate data; it should only compare the data.

Explanation

> java.lang.IllegalArgumentException: Comparison method violates its general contract!

The exception is not thrown from within your try. That is why it is not caught. The exception comes from NotifierHTML.java:363 in your code where you call Collection#sort which uses a TimSort class. The exception is then thrown from TimSort.java:868 by the TimSort#mergeHi method.

It tells you that your implementation of the Comparator#compare method is wrong. It violates the contract, as explained in its documentation:

> Compares its two arguments for order. Returns a negative integer, zero, or a positive integer as the first argument is less than, equal to, or greater than the second. > >The implementor must ensure sgn(x.compareTo(y)) == -sgn(y.compareTo(x)) for all x and y. (This implies that x.compareTo(y) must throw an exception iff y.compareTo(x) throws an exception.) > >The implementor must also ensure that the relation is transitive: (x.compareTo(y) > 0 && y.compareTo(z) > 0) implies x.compareTo(z) > 0. > >Finally, the implementor must ensure that x.compareTo(y) == 0 implies that sgn(x.compareTo(z)) == sgn(y.compareTo(z)), for all z.

Your implementation violates one of those requirements and the method detected that.

---

Source of the problem

The problem is that you return -1 if an error occurs. Suppose you have two values first and second. And that at least one of them will provoke the exception.

So if you want to compare first with second, you get -1:

compare(first, second) -> -1

Which means that first is smaller than second. But if you compare it the other way you get -1 too:

compare(second, first) -> -1

Because the exception is thrown in both variants, which leads to your return -1;. But this means your compare method says:

first < second
second < first

Both at the same time, which is logically incorrect and violates the contract.

---

Solution

You need to correctly define where in your ordering unparsable content is placed at. For example let us define that it is always smaller than any number. So we want

text < number

What do we do if both are unparsable? We could say they are equal, we could compare them lexicographical. Let's keep it simple and say that any two texts are considered equal:

text = text

We implement this by checking which of the arguments are unparseable and then returning the correct value:

@Override
public int compare(Employee first, Employee second) {
    Integer firstValue;
    Integer secondValue;
    try {
        firstValue = Integer.parseInt(first.getBadgeNumber());
    } catch (NumberFormatException e) {
        // Could not parse, set null as indicator
        firstValue = null;
    }
    try {
        secondValue = Integer.parseInt(second.getBadgeNumber());
    } catch (NumberFormatException e) {
        // Could not parse, set null as indicator
        secondValue = null;
    }

    if (firstValue == null && secondValue != null) {
        // text < number
        return -1;
    }
    if (firstValue != null && secondValue == null) {
        // number > text
        return 1;
    }
    if (firstValue == null && secondValue == null) {
        // text = text
        return 0;
    }

    // Both are numbers
    return Integer.compare(firstValue, secondValue);
}

As hinted in the comments you could replace your whole custom Comparator class by the following statement which generates the same Comparator:

Comparator<Employee> comp = Comparator.nullsLast(
    Comparator.comparing(e -> tryParseInteger(e.getBadgeNumber())));

Together with a tryParseInteger method like this:

public static Integer tryParseInteger(String text) {
    try {
        return Integer.parseInt(text);
    } catch (NumberFormatException e) {
        return null;
    }
}

While this is not the case, remember that you can throw and catch Throwable instances, and apart from Exceptions there are Errors. Catching them is possible, though when they occur its unlikely that any further work can be done.

So your try-catch would not have caught an Error or any Throwable other than Exception.

public static void main(String[] args) {

	try {
		throw new Error("test exception try-catch");
	} catch (Throwable e) {
		System.out.println("Error caught in throwable catch");
	}
	
	try {
		throw new Error("test exception try-catch");
	} catch (Exception e) {
		System.out.println("Error caught in exception catch");
	}
}

Which will result in:

Error caught in throwable catch
Exception in thread "main" java.lang.Error: test exception try-catch
    at ...

That exception is not thrown in compare method you pasted here. Check the stacktrace. There is no compare call in it.

The exception is thrown from TimSort.mergeHi() invoked internally as you explicitly invoked Collections.sort() :

> at java.util.TimSort.mergeHi(TimSort.java:868)

You could move the catch statement around sort() but as a consequence the sort will not be performed or be no complete. So it seems not to be a good idea.
Long story short : don't violate the compareTo() contract and you would not need to catch any exception that will not happen any longer.