When catch doesn't actually catch anything

Question

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());

Please post the full stacktrace. Your exception should be caught by that try-catch. So I think the exception comes from elsewhere. And please post a [mcve] (complete). — Zabuzard, May 16 '18 at 19:54
The exception does **not** come from your compare method. It is thrown by the sorter method which detects that your implementation of `compareTo` is wrong. You need to implement it like it states in its documentation. That is, negative if first is less than second, positive if first is greater than second and `0` if equal. — Zabuzard, May 16 '18 at 19:55
Is your question about the exception hierarchy and the fact that you thought you were handling the exception and it is not, or about the fact that you get an exception while you weren't expecting it? — M. le Rutte, May 16 '18 at 19:59
See also: https://stackoverflow.com/questions/8327514/comparison-method-violates-its-general-contract — Ilmari Karonen, May 17 '18 at 07:09
Moral of the story: don't just blindly catch exceptions where you can't deal with them and still behave correctly. If you *must* deal with non-integer `BadgeNumber`s, they will have to have a proper place in your ordering (i.e. either larger than all numbers or smaller than all numbers) — Caleth, May 17 '18 at 08:26
If you want to do it this way, you should return a positive number if the one call throws an exception, a negative number if the other call throws one and 0 if both throw exceptions. — Bernhard Barker, May 17 '18 at 10:34
Attaching a debugger at the catch block would’ve shown that the exception is indeed caught. You might also want to validate for null input - only the first is null (-1) or only the second is null (1) or when both are null (0) — Timir, May 17 '18 at 16:11
@Zabuza That is not "your implementation of `compareTo`", that is `java.lang.Integer.compareTo`. — Laurenz Albe, May 18 '18 at 06:58
This question would've been much more useful with a [mcve]. As it stands it's based on a misunderstanding about where the exception is being thrown, which makes it much less unlikely to be helpful to anyone else. Consider that you can get the same exception with an if-statement instead of a try-catch, which would have exactly the same answer for why you're getting the exception and how to fix it. Also, this misunderstanding means the answers need to address that instead of being more general to address similar but not identical questions. Sounds close-worthy - not sure why it was reopened. — Bernhard Barker, May 18 '18 at 07:41

score 143 · Accepted Answer · answered May 16 '18 at 19:58

143

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.

answered May 16 '18 at 19:58

rgettman

176,041
30
275
357

4

They really should have added a new RuntimeException for this as it is pretty misleading as it stands. `ContractException` would have been nice and could have been used more generally. – davidbak May 16 '18 at 23:15
2

@davidbak Who says that `IllegalArgumentException` only applies to the *data* of the argument, not the *behaviour*? And how would you enforce such seperation – Caleth May 17 '18 at 08:24
@JimGarrison if I had to hazard a guess, someone read the first sentence, dereferenced "the exception" to the one whose stacktrace was posted, and cast a downvote without reading the rest. – Haem May 17 '18 at 09:24
"because the exception will be caught in both cases" How can java be sure of that? What if I want to raise and catch an exception when a < b, and instead make everything work normally if a > b? This seems weird to me. Why try to enforce something like this? Let the programmer worry about it – Ant May 17 '18 at 13:41
2

@Ant You're missing the point. The error is thrown because the definition of ```<``` and ```>```given to Java here are themselves found to be contradictory, in a manner that might leave the sorting stuck forever. – Haem May 17 '18 at 14:08
@Haem Ah, I see, thank you. I am curious to know how java is able to tell that the definition is inconsistent though. – Ant May 17 '18 at 14:19
@Caleth Nothing's enforced about the "name" of an exception vs the reason it is thrown, any more than anything is enforced that says a "Supplier" functional interface has to supply anything or a "Consumer" functional interface has to consume anything. It's just a convention that helps the programmer make sense of things. And, by convention, names _and documentation_ describe the intended use (as well as semantics) so consider that [the official doc](https://docs.oracle.com/javase/9/docs/api/java/lang/IllegalArgumentException.html) talks about _arguments_ and not _behavior_. – davidbak May 17 '18 at 16:32
@Ant as mentioned in the answer, that's only occasionally caught, such as when the sort runs into a situation where ```a – Haem May 17 '18 at 18:00
2

@davidbak, presumably, the `compare` function is being provided as an argument to `sort()`. Since `sort` requires its comparison function to be well-behaved, an inconsistent comparison is an invalid argument to the function. – Mark May 17 '18 at 20:37

score 53 · Answer 2 · edited May 18 '18 at 10:34

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;
    }
}

Just a minor comment: I think that the comparator could be written as `Comparator.nullsLast(Comparator.comparing(e -> tryParseInteger(e.getBadgeNumber())))` (with a method `tryParseInteger` implemented accordingly) — Marco13, May 17 '18 at 12:36
@Marco13 Given that I was cursing the needless verbosity of Java while reading this at first, I think your solution is an important addition. — JollyJoker, May 17 '18 at 13:51
This explains the **actual** problem... that the sort function is inconsistent. — Salman A, May 18 '18 at 10:33
Unrelated: I was just about to edit my answer and add that javadoc, when the OP deleted the question. Grmpf. I could have made good use of some upvotes/accepts today. Would have been a nice closure for a day when I saw ... as so often, questions of my own downvoted for no apparent reasons ... — GhostCat, Sep 04 '18 at 19:28

score 4 · Answer 3 · answered May 17 '18 at 08:05

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 ...

Correct, and every Java programmer should be aware of that, but ... unrelated to the question.... — Marco13, May 17 '18 at 09:39
It's related to the title, which is the most indexed and googlable element. Take a look at [my other answer](https://stackoverflow.com/questions/20427210/mybatis-spring-mvc-application-getting-invalid-bound-statement-not-found/31533704#31533704) which is also unrelated, but that's what ppl search for and go there. — Dariusz, May 17 '18 at 10:53

score 2 · Answer 4 · answered May 16 '18 at 20:04

2

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

answered May 16 '18 at 20:04

Antoniossss

31,590
6
57
99

score 0 · Answer 5 · answered May 16 '18 at 19:58

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.

When catch doesn't actually catch anything

5 Answers5

Explanation

Source of the problem

Solution