Opened 8 years ago

Closed 8 years ago

#811 closed defect (fixed)

B0rked in the tunnel builder: java.lang.IllegalArgumentException

Reported by: killyourtv Owned by: zzz
Priority: minor Milestone: 0.9.5
Component: router/general Version: 0.9.3
Keywords: Cc: Zlatin Balevsky
Parent Tickets: Sensitive: no


I2P version: 0.9.3-15
Java version: Oracle Corporation 1.7.0_03 (OpenJDK Runtime Environment 1.7.0_03-b21)
Wrapper version: 3.5.16
Server version: 6.1.26
Servlet version: Jasper JSP 2.1 Engine
Platform: Linux amd64 3.2.0-4-amd64

A few minutes after starting, I had the following in my logs:

12/15/12 22:13:44.755 CRIT [uildExecutor] uter.tunnel.pool.BuildExecutor: B0rked in the tunnel builder
     java.lang.IllegalArgumentException: Comparison method violates its general contract!
     at java.util.TimSort.mergeHi(
     at java.util.TimSort.mergeAt(
     at java.util.TimSort.mergeCollapse(
     at java.util.TimSort.sort(
     at java.util.TimSort.sort(
     at java.util.Arrays.sort(
     at java.util.Collections.sort(


Change History (6)

comment:2 Changed 8 years ago by Zlatin Balevsky

Cc: Zlatin Balevsky added

The BuildExecutor.TunnelPoolComparator looks fine unless the return values of getTunnelCount() or getSettings().isExploratory() can change while the list is being sorted. (I think? Can anyone find a case that breaks total order?)

Either set the property java.util.Arrays.useLegacyMergeSort=true to mute the exception (but risk that the wanted list may not always be sorted correctly) or apply http://pastethis.i2p/show/2488/

comment:3 Changed 8 years ago by zzz

Owner: set to zzz
Status: newaccepted

Agreed with your analysis. Only on Java 7. Minor because the BuildExecutor? will loop and try again.

Occasional mis-sorting in BuildExecutor? is not a big problem. However, Oracle claims a big speedup with TimSort? so I'd rather not set the property for the whole JVM.

Your paste 2488 may be overkill? My quick and dirty idea for a fix:

        try {
           Collections.sort(wanted, new TunnelPoolComparator());
        } catch (IllegalArgumentException iae) {

comment:4 Changed 8 years ago by Zlatin Balevsky

For now it should work. You're opening yourself up to future changes in the code inside the try clause that could throw an IAE that you do not want to ignore.

comment:5 Changed 8 years ago by zzz

I neglected to answer your earlier question. isExploratory() will not change but getTunnelCount() may.

comment:6 Changed 8 years ago by zzz

Resolution: fixed
Status: acceptedclosed

Patch similar to that in comment 3 above applied in 0.9.4-2.

Note: See TracTickets for help on using tickets.