53

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());
10
  • 5
    IllegalArgumentException does. May 16, 2018 at 19:52
  • 7
    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 minimal reproducible example (complete).
    – Zabuzard
    May 16, 2018 at 19:54
  • 34
    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, 2018 at 19:55
  • 4
    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 BadgeNumbers, 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, 2018 at 8:26
  • 6
    This question would've been much more useful with a minimal reproducible example. 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. May 18, 2018 at 7:41

5 Answers 5

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.

9
  • 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, 2018 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, 2018 at 8: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, 2018 at 9:24
  • 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, 2018 at 14:08
  • 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, 2018 at 20:37
53

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;
    }
}
5
  • 7
    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, 2018 at 12:36
  • @Marco13 Sounds like it would work, nice spot!
    – Zabuzard
    May 17, 2018 at 13:11
  • 1
    @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, 2018 at 13:51
  • 1
    This explains the actual problem... that the sort function is inconsistent.
    – Salman A
    May 18, 2018 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 4, 2018 at 19:28
4

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 ...
2
  • 3
    Correct, and every Java programmer should be aware of that, but ... unrelated to the question....
    – Marco13
    May 17, 2018 at 9:39
  • It's related to the title, which is the most indexed and googlable element. Take a look at my other answer which is also unrelated, but that's what ppl search for and go there.
    – Dariusz
    May 17, 2018 at 10:53
2

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

0

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.

Not the answer you're looking for? Browse other questions tagged or ask your own question.