[CORE] orderbook recheckidx fix

xchange-core/src/main/java/org/knowm/xchange/dto/marketdata/OrderBook.java

@@ -142,19 +142,24 @@ private void update(List<LimitOrder> limitOrders, LimitOrder limitOrder) {
         long writeStamp = lock.tryConvertToWriteLock(stamp);
         if (writeStamp != 0L) {
           stamp = writeStamp;
-          if (idx >= 0) limitOrders.remove(idx);
-          else idx = -idx - 1;
-          if (limitOrder.getRemainingAmount().compareTo(BigDecimal.ZERO) != 0)
+          if (idx >= 0) {
+            limitOrders.remove(idx);
+          } else {
+            idx = -idx - 1;
+          }
+          if (limitOrder.getRemainingAmount().compareTo(BigDecimal.ZERO) != 0) {
             limitOrders.add(idx, limitOrder);
+          }
           updateDate(limitOrder.getTimestamp());
           break;
         } else {
           lock.unlockRead(stamp);
           stamp = lock.writeLock();
           // here wee need to recheck idx, because it is possible that orderBook changed between
-          // unlockRead and lockWrite
-          if (recheckIdx(limitOrders, limitOrder, idx))
+          // unlockRead and writeLock
+          if (recheckIdx(limitOrders, limitOrder, idx)) {
             idx = Collections.binarySearch(limitOrders, limitOrder);
+          }
         }
       }
     } finally {
@@ -179,8 +184,11 @@ public void update(OrderBookUpdate orderBookUpdate) {
         long writeStamp = lock.tryConvertToWriteLock(stamp);
         if (writeStamp != 0L) {
           stamp = writeStamp;
-          if (idx >= 0) limitOrders.remove(idx);
-          else idx = -idx - 1;
+          if (idx >= 0) {
+            limitOrders.remove(idx);
+          } else {
+            idx = -idx - 1;
+          }
           if (orderBookUpdate.getTotalVolume().compareTo(BigDecimal.ZERO) != 0) {
             LimitOrder updatedOrder = withAmount(limitOrder, orderBookUpdate.getTotalVolume());
             limitOrders.add(idx, updatedOrder);
@@ -192,31 +200,36 @@ public void update(OrderBookUpdate orderBookUpdate) {
           stamp = lock.writeLock();
           // here wee need to recheck idx, because it is possible that orderBook changed between
           // unlockRead and lockWrite
-          if (recheckIdx(limitOrders, limitOrder, idx))
+          if (recheckIdx(limitOrders, limitOrder, idx)) {
             idx = Collections.binarySearch(limitOrders, limitOrder);
+          }
         }
       }
     } finally {
       lock.unlock(stamp);
     }
   }
 
+  /**
+   * @return true, if wee need to run binarySearch again
+   */
   private boolean recheckIdx(List<LimitOrder> limitOrders, LimitOrder limitOrder, int idx) {
-    if (idx >= 0) {
-      // if positive, null check or compare
-      return limitOrders.get(idx) == null || limitOrders.get(idx).compareTo(limitOrder) != 0;
-    } else {
-      // on end of array, null check or one check
-      if (limitOrders.size() == -idx - 1) {
-        return limitOrders.get(-idx - 2) == null
-            || limitOrders.get(-idx - 2).compareTo(limitOrder) >= 0;
-      } else
-        // if negative, check that of limitOrders.get(reversed idx) limitOrders.get(reversed idx-1)
-        // and is lower and bigger than limitOrder
-        return (limitOrders.get(-idx - 1) == null
-                || limitOrders.get(-idx - 1).compareTo(limitOrder) <= 0)
-            && (limitOrders.get(-idx - 2) == null
-                || limitOrders.get(-idx - 2).compareTo(limitOrder) >= 0);
+    switch (idx) {
+      case 0:
+        {
+          if (!limitOrders.isEmpty()) {
+            // if not equals, need to recheck
+            return limitOrders.get(0).compareTo(limitOrder) != 0;
+          } else return true;
+        }
+      case -1:
+        {
+          if (limitOrders.isEmpty()) {
+            return false;
+          } else return limitOrders.get(0).compareTo(limitOrder) <= 0;
+        }
+      default:
+        return true;
     }
   }
 

xchange-core/src/test/java/org/knowm/xchange/dto/marketdata/ConcurrencyTest.java

@@ -19,12 +19,10 @@ public class ConcurrencyTest {
   static Instrument inst = new CurrencyPair("BTC/USDT");
 
   public static void main(String[] args) throws InterruptedException, ExecutionException {
-    OrderBook orderBook1 =
-        new OrderBook(new Date(), initOrderBookAsks(), initOrderBookBids(), true);
-    OrderBook orderBook2 =
-        new OrderBook(new Date(), initOrderBookAsks(), initOrderBookBids(), true);
+    OrderBook orderBook1 = new OrderBook(new Date(), initOrderBookAsks(), new ArrayList<>(), true);
+    OrderBook orderBook2 = new OrderBook(new Date(), initOrderBookAsks(), new ArrayList<>(), true);
     OrderBookOld orderBookOld =
-        new OrderBookOld(new Date(), initOrderBookAsks(), initOrderBookBids(), true);
+        new OrderBookOld(new Date(), initOrderBookAsks(), new ArrayList<>(), true);
     ThreadPoolExecutor executor = (ThreadPoolExecutor) Executors.newFixedThreadPool(50);
     newWay(orderBook1, executor);
     executor.awaitTermination(100L, TimeUnit.SECONDS);
@@ -114,7 +112,7 @@ private static void oldOB(OrderBookOld orderBookOld, ThreadPoolExecutor executor
 
   public static void updateOrderBook1(OrderBook orderBook, boolean oldWay) {
     Random rand = new Random(123);
-    for (int i = 0; i < 100000; i++) {
+    for (int i = 0; i < 10000; i++) {
       OrderBookUpdate orderBookUpdateAsk =
           new OrderBookUpdate(
               OrderType.ASK,
@@ -145,7 +143,7 @@ public static void updateOrderBook1(OrderBook orderBook, boolean oldWay) {
 
   public static void updateOrderBookOld1(OrderBookOld orderBookOld) {
     Random rand = new Random(123);
-    for (int i = 0; i < 100000; i++) {
+    for (int i = 0; i < 10000; i++) {
       OrderBookUpdate orderBookUpdateAsk =
           new OrderBookUpdate(
               OrderType.ASK,
@@ -171,7 +169,7 @@ public static void updateOrderBookOld1(OrderBookOld orderBookOld) {
 
   private static void updateOrderBook2(OrderBook orderBook, boolean oldWay) {
     Random rand = new Random(123);
-    for (int i = 0; i < 100000; i++) {
+    for (int i = 0; i < 10000; i++) {
       LimitOrder bookUpdateAsk =
           new LimitOrder(
               OrderType.ASK,
@@ -202,7 +200,7 @@ private static void updateOrderBook2(OrderBook orderBook, boolean oldWay) {
 
   private static void updateOrderBookOld2(OrderBookOld orderBookOld) {
     Random rand = new Random(123);
-    for (int i = 0; i < 100000; i++) {
+    for (int i = 0; i < 10000; i++) {
       LimitOrder bookUpdateAsk =
           new LimitOrder(
               OrderType.ASK,
@@ -227,7 +225,7 @@ private static void updateOrderBookOld2(OrderBookOld orderBookOld) {
   }
 
   private static void readOrderBook(OrderBook orderBook, boolean oldWay) {
-    for (int i = 0; i < 1200000; i++) {
+    for (int i = 0; i < 10000; i++) {
       int temp = 0;
       if (oldWay) {
         synchronized (orderBook) {
@@ -252,7 +250,7 @@ private static void readOrderBook(OrderBook orderBook, boolean oldWay) {
   }
 
   private static void readOrderBookOld(OrderBookOld orderBookOld) {
-    for (int i = 0; i < 1200000; i++) {
+    for (int i = 0; i < 120000; i++) {
       int temp = 0;
       synchronized (orderBookOld) {
         for (LimitOrder ask : orderBookOld.getAsks()) {

xchange-core/src/test/java/org/knowm/xchange/dto/marketdata/OrderBookTest.java

@@ -3,6 +3,8 @@
 import static org.assertj.core.api.Assertions.assertThat;
 
 import java.io.IOException;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
 import java.math.BigDecimal;
 import java.time.Duration;
 import java.util.ArrayList;
@@ -174,4 +176,43 @@ public void testOrderSorting() {
     assertThat(book.getAsks().get(0).getLimitPrice()).isEqualTo(new BigDecimal("10"));
     assertThat(book.getBids().get(0).getLimitPrice()).isEqualTo(new BigDecimal("3"));
   }
+
+  @Test
+  public void testRecheckIdx()
+      throws IOException, NoSuchMethodException, InvocationTargetException, IllegalAccessException {
+    Class[] cArg = new Class[3];
+    cArg[0] = List.class;
+    cArg[1] = LimitOrder.class;
+    cArg[2] = int.class;
+    Method method = OrderBook.class.getDeclaredMethod("recheckIdx", cArg);
+    method.setAccessible(true);
+    LimitOrder sameBidOrder =
+        new LimitOrder(
+            OrderType.BID, BigDecimal.ONE, CurrencyPair.BTC_USD, "", null, BigDecimal.TEN);
+    LimitOrder smallerBidOrder =
+        new LimitOrder(
+            OrderType.BID, BigDecimal.ONE, CurrencyPair.BTC_USD, "", null, BigDecimal.ONE);
+    LimitOrder higherBidOrder =
+        new LimitOrder(
+            OrderType.BID, BigDecimal.ONE, CurrencyPair.BTC_USD, "", null, new BigDecimal("10.5"));
+    // idx!= -1,0
+    assertThat(method.invoke(orderBook, new ArrayList<LimitOrder>(), sameBidOrder, 1))
+        .isEqualTo(true);
+    // idx=0, empty List
+    assertThat(method.invoke(orderBook, new ArrayList<LimitOrder>(), sameBidOrder, 0))
+        .isEqualTo(true);
+    // idx=0, order equals
+    assertThat(method.invoke(orderBook, orderBook.getBids(), sameBidOrder, 0)).isEqualTo(false);
+    // idx=0, smallerBidOrder
+    assertThat(method.invoke(orderBook, orderBook.getBids(), smallerBidOrder, 0)).isEqualTo(true);
+    // idx=-1, empty List
+    assertThat(method.invoke(orderBook, new ArrayList<LimitOrder>(), sameBidOrder, -1))
+        .isEqualTo(false);
+    // idx=-1, order equals
+    assertThat(method.invoke(orderBook, orderBook.getBids(), sameBidOrder, -1)).isEqualTo(true);
+    // idx=-1, smaller order
+    assertThat(method.invoke(orderBook, orderBook.getBids(), smallerBidOrder, -1)).isEqualTo(true);
+    // idx=-1, higher order
+    assertThat(method.invoke(orderBook, orderBook.getBids(), higherBidOrder, -1)).isEqualTo(false);
+  }
 }