waitForFill looses track of modified order

[wwadge]

Hi,

Thanks for the great project :-)

In waitForFill method, there's the following:

Order updated = updateOrder(order, null);
if (updated.isFinalized()) {
             return;
}

We are in a thread there so the modified updated value (the one with a new order ID when an order gets filled) never gets sent back to the caller. Because of this, the executeOrder in AccountManager never gets the updated order so when it gets the FILLED notification, it fails to remove the updated order from the pending queue.

I'm not quite sure how best to fix this, it's easy to just say if (updated.isFinalized()) {... remove from queue) in the snippet above but the actual object itself is not being updated elsewhere so for example if it needs to get logged it won't have the "latest and greatest" values. An alternate way would be to set the order handle properties to the new one but that means we've lost immutability.

[wwadge]

Actually with my previous patch it shouldn't matter, however there's another bug. Since we're looping, we need to keep passing in the latest updated order: Order updated = updateOrder(order, null);

here we keep passing the original order instead of the updated one. I'll prepare a MR

[jbax]

Thanks I just merged your PR. I didn't have time to test this out properly since I'm working on other stuff. I'll leave this issue open for review until I'm able to run the live trading bit and check. Thanks again for your help!

[wwadge]

(Adding here for visibility)

I'm trying to track down the following bug. So first I had an order that was filled:

INFO (BinanceClientAccount.java:184) - Executing LIMIT order: NewOrder[] TRACE (AccountManager.java:707) - Tracking pending order. NEW LIMIT SELL of .... [Order 111953750 monitor:SELL ADAETH] TRACE (AccountManager.java:707) - FILLED LIMIT SELL ...

but a short while later I get: [nioEventLoopGroup-2-2] ERROR (AccountManager.java:864) - Failed to execute cancellation of order 'CANCELLED LIMIT BUY......' on exchange com.univocity.trader.exchange.binance.api.client.exception.BinanceApiException: Unknown order sent. at com.univocity.trader.exchange.binance.api.client.impl.BinanceApiServiceGenerator.executeSync(BinanceApiServiceGenerator.java:87) at com.univocity.trader.exchange.binance.api.client.impl.BinanceApiRestClientImpl.cancelOrder(BinanceApiRestClientImpl.java:135) at com.univocity.trader.exchange.binance.BinanceClientAccount.cancel(BinanceClientAccount.java:214) at com.univocity.trader.account.AccountManager.cancelOrder(AccountManager.java:862) at com.univocity.trader.account.AccountManager.cancelOrder(AccountManager.java:882) at com.univocity.trader.account.TradingManager.cancelOrder(TradingManager.java:253) at com.univocity.trader.account.Trader.exit(Trader.java:247) at com.univocity.trader.account.Trader.processSell(Trader.java:224) at com.univocity.trader.account.Trader.trade(Trader.java:162) at com.univocity.trader.strategy.TradingEngine.process(TradingEngine.java:84) at com.univocity.trader.candles.CandleProcessor.processCandle(CandleProcessor.java:30)

Now here's what I think is wrong, I opened a somewhat related issue on this already but there's something else: The waitForFill method has:

updated = updateOrder(updated, null);
if (updated.isFinalized()) {
   return;
}

but note that this updated Order (with a new ID) doesn't get bubbled up to the caller. Indeed the caller would have already gone away and provided the original order to callees. Thus I think we are left that in the Trader.exit() method, we are trying to cancel an order with an ID that has become obsolete. I can think of two ways to fix this but I don't know enough about the architecture:

1) Either call order.setId(updated.getId()) since we still have the original handle. This implies removing final and hoping no one has done a copy of the Order instance 2) Make the callee wait for the thread instead. This would be "safer" but I don't know if it would block up the rest of the event flow.

[wwadge]

The first option is problematic, just because we change the orderId does not imply the OrderedSet will be resorted -- perhaps switch to a LinkedHashSet instead ?

What happens in option 2? In other words, what happens if waitForFill becomes a blocking call?

[wwadge]

Fix attempted here: https://github.com/uniVocity/univocity-trader/pull/43

[jbax]

Ok so I think I've fixed the error on my latest commit. What you have been seeing was simply an effect of lack of synchronization among threads. The OrderSet and TradeSet are not thread safe on purpose to ensure simulations run as fast as possible. I introduced these to replace the slower java concurrent/synchronized collections but I did not care about synchronization at the time (it was not the focus for simulations).

So I came up with a solution that can run without locking when simulating, and that takes care of synchronization when live trading (where multiple threads manipulate the OrderSet and TradeSet instances) with a fake implementation of java.util.concurrent.Lock for simulations and a proper ReentrantLock for live trading. Seems to be working fine now.

Let me know if this fixes all the issues you had. I'll keep this issue open until you can confirm. Thanks for digging into this!

[wwadge]

Pulling this and will test - it can take a long-ish while to trigger though

[wwadge]

Seems to have worked. Thanks!