Prevent the bot to enter an unknown state when receiving a 5XX response for a getOpenOrders request #287

Merged

dazito merged 4 commits from fetchOpenOrder-catch-all-exceptions into master

2020-12-18 12:13:36 -08:00

dazito commented

2020-11-29 03:26:35 -08:00

(Migrated from github.com)

This happened to me when the exchange returned 503 the bot kept running and but I wasn't sure if the bot was aware there were open orders. This will prevent it and force the bot to request again the open orders from the exchange

scionaltera (Migrated from github.com) reviewed

2020-11-29 22:35:43 -08:00

src/main/java/com/r307/arbitrader/service/TradingService.java

I'd like to avoid catching the top-level Exception as much as possible. It can lead to unintended behavior if an exception is thrown that you weren't planning for. If the exchange threw a different exception let's just add that specific one to this catch if the catch is the correct action to take. Otherwise I'd prefer to let unexpected runtime exceptions bubble up to the top and crash the program. That way we are sure to notice it, we can figure out where it came from and decide what to do with it.

I'd like to avoid catching the top-level `Exception` as much as possible. It can lead to unintended behavior if an exception is thrown that you weren't planning for. If the exchange threw a different exception let's just add that specific one to this catch if the catch is the correct action to take. Otherwise I'd prefer to let unexpected runtime exceptions bubble up to the top and crash the program. That way we are sure to notice it, we can figure out where it came from and decide what to do with it.

dazito (Migrated from github.com) reviewed

2020-11-30 11:46:41 -08:00

src/main/java/com/r307/arbitrader/service/TradingService.java

As far as I understand, the do ... while cycle is there to prevent the bot to proceed while the orders are not all set/filled. Or am I wrong here?

If I am correct, then I think we should catch any exception in the this part of executeOrderPair logic. If not here then in the do ... while cycle because If an exception is not catched we may run into an incorrect state: trying to exit an order that was not yet filled.

For example, we execute an order that is not instantly filled. Then in the do ... while() cycle we keep checking whether the order is filled or not before proceeding. If we get an exception here and it is not catched then the bot will stop proceeding any further in the executeOrderPair logic. Then the bot will start to find an exit trade but here we have two possible cases:

The order was actually filled and everything is fine ✔️
The order is still waiting to be filled but because the exception was not catched the bot is not aware of it (entry order not yet filled) and will try to exit an order that was not filled yet ❌

As far as I understand, the `do ... while` cycle is there to prevent the bot to proceed while the orders are not all set/filled. Or am I wrong here? If I am correct, then I think we should catch any exception in the this part of `executeOrderPair` logic. If not here then in the `do ... while` cycle because If an exception is not catched we may run into an incorrect state: trying to exit an order that was not yet filled. For example, we execute an order that is not instantly filled. Then in the `do ... while()` cycle we keep checking whether the order is filled or not before proceeding. If we get an exception here and it is not catched then the bot will stop proceeding any further in the `executeOrderPair` logic. Then the bot will start to find an exit trade but here we have two possible cases: 1. The order was actually filled and everything is fine :heavy_check_mark: 2. The order is still waiting to be filled but because the exception was not catched the bot is not aware of it (entry order not yet filled) and will try to exit an order that was not filled yet :x:

scionaltera (Migrated from github.com) reviewed

2020-11-30 12:25:49 -08:00

src/main/java/com/r307/arbitrader/service/TradingService.java

Yes, the sleep() loop is there to make the bot wait for the limit orders to fill. There is nothing else the bot needs to be doing at that time since we only do one pair of orders at a time.

The contract of fetchOpenOrders() is that it is not supposed to throw any exceptions. If it does, that means something went wrong. The exception is not caught in executeOrderPair() so it will bubble up to entryPosition() or exitPosition(). If it is an IOException subclass it will be caught and handled (not well enough) there. If not, it bubbles all the way up to the top and crashes the program. I think this is the correct behavior for an unexpected exception: we don't know what happened or what could have happened as a result of it. We need a human to review it and fix it.

I'm open to adding additional handlers for specific exceptions where we know what happened and how to reliably get the bot back into a good state. Any time there is uncertainty, especially when dealing with money, I think it's best to fail fast and let the human fix it.

Yes, the `sleep()` loop is there to make the bot wait for the limit orders to fill. There is nothing else the bot needs to be doing at that time since we only do one pair of orders at a time. The contract of `fetchOpenOrders()` is that it is not supposed to throw any exceptions. If it does, that means something went wrong. The exception is not caught in `executeOrderPair()` so it will bubble up to `entryPosition()` or `exitPosition()`. If it is an `IOException` subclass it will be caught and handled (not well enough) there. If not, it bubbles all the way up to the top and crashes the program. I think this is the correct behavior for an **unexpected** exception: we *don't know what happened* or what could have happened as a result of it. We need a human to review it and fix it. I'm open to adding additional handlers for **specific** exceptions where we know what happened and how to *reliably* get the bot back into a good state. Any time there is uncertainty, especially when dealing with money, I think it's best to fail fast and let the human fix it.

dazito (Migrated from github.com) reviewed

2020-11-30 13:21:25 -08:00

src/main/java/com/r307/arbitrader/service/TradingService.java

I understand your point and I agree but in my case the thrown exception was ExchangeException which is a RuntimeException and a generic Exchange exception. In my case, the exception was thrown but the bot did not crash. Instead, for some reason, it kept running. With this PR I wanted to at least avoid the bot to keep running.

Should it catch a ExchangeException instead? XChange documentation says it is a Indication of generic Exchange exception

Should it be kept as it is? If so feel free to close this PR

I understand your point and I agree but in my case the thrown exception was `ExchangeException` which is a `RuntimeException` and a generic Exchange exception. In my case, the exception was thrown but the bot did not crash. Instead, for some reason, it kept running. With this PR I wanted to at least avoid the bot to keep running. Should it catch a `ExchangeException` instead? XChange documentation says it is a `Indication of generic Exchange exception` Should it be kept as it is? If so feel free to close this PR

scionaltera (Migrated from github.com) reviewed

2020-12-13 20:53:07 -08:00

src/main/java/com/r307/arbitrader/service/TradingService.java

Sorry for the delay on this and the other PRs. Between some craziness in my personal life and my bot not getting any trades while I'm trying to test a branch, I've been pretty blocked.

I think if we are getting an ExchangeException unexpectedly that is both not crashing the bot and not handled in a way that we understand, maybe we should catch it in the same block as the IOException and have IOException | ExchangeException. In either case we don't really have a clear idea (programmatically) what the problem is or what we should do to fix it, so we should log and bail out.

Sorry for the delay on this and the other PRs. Between some craziness in my personal life and my bot not getting any trades while I'm trying to test a branch, I've been pretty blocked. I think if we are getting an `ExchangeException` unexpectedly that is both not crashing the bot and not handled in a way that we understand, maybe we should catch it in the same block as the `IOException` and have `IOException | ExchangeException`. In either case we don't really have a clear idea (programmatically) what the problem is or what we should do to fix it, so we should log and bail out.