Repository: liampauling/flumine (python)

Issue:
Wrong order state after multiple connection reset errors
Hi!

I sometimes get multiple `ConnectionResetError`s when trying to cancel an order, even when retrying. After the last failed attempt, the order state is not properly reset, but remains `CANCELLING`.

I looked at the code to execute the cancel operation:
https://github.com/liampauling/flumine/blob/4da58825ae529ddde2b3bfee2a434b20ed41ec3c/flumine/execution/betfairexecution.py#L55

The actual exception is handled here:
https://github.com/liampauling/flumine/blob/4da58825ae529ddde2b3bfee2a434b20ed41ec3c/flumine/execution/betfairexecution.py#L244

However, after the last attempt, there is simply no new retry and the order is not updated again to reflect the failed request.
Maybe the exception handler should re-raise the error when not retrying the operation. Then, the `execute_cancel` method could catch the error and set the order state back to `EXECUTABLE`, when the state is still `CANCELLING` (to *mostly* avoid the current problem with race conditions in the order state processing).

Also: I noticed flumine is using its own session pool instead of relying on the connection pool of the session itself. Why is that?

Pull request:
wrong order state after multiple connection reset errors
closes #464

Patch:
diff --git a/HISTORY.rst b/HISTORY.rst
index 3839ad2..ba5e507 100644
--- a/HISTORY.rst
+++ b/HISTORY.rst
@@ -12,6 +12,7 @@ Release History
 **Bug Fixes**
 
 - #454 SP nr size adjustment (@jsphon)
+- #464 wrong order state after multiple connection reset errors
 
 1.19.6 (2021-07-09)
 +++++++++++++++++++
diff --git a/flumine/execution/betfairexecution.py b/flumine/execution/betfairexecution.py
index 2c9d609..ca99fed 100644
--- a/flumine/execution/betfairexecution.py
+++ b/flumine/execution/betfairexecution.py
@@ -253,6 +253,12 @@ class BetfairExecution(BaseExecution):
                 )
                 if order_package.retry():
                     self.handler(order_package)
+                else:
+                    # reset orders
+                    if order_package.package_type == OrderPackageType.PLACE:
+                        order_package.reset_orders(complete=True)
+                    else:
+                        order_package.reset_orders()
 
                 self._return_http_session(http_session, err=True)
                 return
diff --git a/flumine/markets/middleware.py b/flumine/markets/middleware.py
index 38c5e85..bafe57c 100644
--- a/flumine/markets/middleware.py
+++ b/flumine/markets/middleware.py
@@ -117,13 +117,6 @@ class SimulatedMiddleware(Middleware):
                                 / (100 - runner_adjustment_factor)
                             )
                             order.order_type.liability *= multiplier
-                            if order.average_price_matched:
-                                # We will get here if the NR is declared inplay
-                                order.current_order.size_matched = round(
-                                    order.order_type.liability
-                                    / (order.average_price_matched - 1),
-                                    2,
-                                )
                             logger.warning(
                                 "WIN MARKET_ON_CLOSE Order adjusted due to non runner {0}".format(
                                     order.selection_id
@@ -133,13 +126,6 @@ class SimulatedMiddleware(Middleware):
                         elif market.market_type in {"PLACE", "OTHER_PLACE"}:
                             multiplier = (100 - removal_adjustment_factor) * 0.01
                             order.order_type.liability *= multiplier
-                            if order.average_price_matched:
-                                # We will get here if the NR is declared inplay
-                                order.current_order.size_matched = round(
-                                    order.order_type.liability
-                                    / (order.average_price_matched - 1),
-                                    2,
-                                )
                             logger.warning(
                                 "PLACE MARKET_ON_CLOSE Order adjusted due to non runner {0}".format(
                                     order.selection_id
diff --git a/flumine/order/orderpackage.py b/flumine/order/orderpackage.py
index 046cccc..ebf8cad 100644
--- a/flumine/order/orderpackage.py
+++ b/flumine/order/orderpackage.py
@@ -61,6 +61,14 @@ class BaseOrderPackage(BaseEvent):
             return True
         return False
 
+    def reset_orders(self, complete: bool = False) -> None:
+        for order in self:
+            with order.trade:
+                if complete:
+                    order.execution_complete()
+                else:
+                    order.executable()
+
     def calc_simulated_delay(self) -> float:
         if self.client.execution.EXCHANGE == ExchangeType.SIMULATED:
             if self.package_type == OrderPackageType.PLACE:


Test patch:
diff --git a/tests/test_execution.py b/tests/test_execution.py
index 814a12d..cf5e2f0 100644
--- a/tests/test_execution.py
+++ b/tests/test_execution.py
@@ -833,6 +833,33 @@ class BetfairExecutionTest(unittest.TestCase):
         mock_trading_function.assert_called_with(mock_order_package, mock_session)
         mock__return_http_session.assert_called_with(mock_session, err=True)
         mock_handler.put.assert_not_called()
+        mock_order_package.reset_orders.assert_called_with()
+
+    @mock.patch("flumine.execution.betfairexecution.BetfairExecution.handler")
+    @mock.patch(
+        "flumine.execution.betfairexecution.BetfairExecution._return_http_session"
+    )
+    def test__execution_helper_error_no_retry_place(
+        self, mock__return_http_session, mock_handler
+    ):
+        mock_trading_function = mock.Mock()
+        mock_trading_function.__name__ = "test"
+        mock_trading_function.side_effect = BetfairError()
+        mock_session = mock.Mock()
+        mock_order_package = mock.Mock(
+            elapsed_seconds=0.001, package_type=OrderPackageType.PLACE
+        )
+        mock_order_package.info = {}
+        mock_order_package.retry.return_value = False
+        self.assertIsNone(
+            self.execution._execution_helper(
+                mock_trading_function, mock_order_package, mock_session
+            )
+        )
+        mock_trading_function.assert_called_with(mock_order_package, mock_session)
+        mock__return_http_session.assert_called_with(mock_session, err=True)
+        mock_handler.put.assert_not_called()
+        mock_order_package.reset_orders.assert_called_with(complete=True)
 
     @mock.patch(
         "flumine.execution.betfairexecution.BetfairExecution._return_http_session"
diff --git a/tests/test_middleware.py b/tests/test_middleware.py
index 3fcc046..4cc68bd 100644
--- a/tests/test_middleware.py
+++ b/tests/test_middleware.py
@@ -140,13 +140,7 @@ class SimulatedMiddlewareTest(unittest.TestCase):
     def test__process_runner_removal_sp_win(self):
         order_type = MarketOnCloseOrder(liability=200)
         mock_order = mock.Mock(
-            selection_id=1234,
-            handicap=0,
-            order_type=order_type,
-            info={},
-            side="LAY",
-            current_order=mock.Mock(size_matched=0),
-            average_price_matched=None,
+            selection_id=1234, handicap=0, order_type=order_type, info={}, side="LAY"
         )
 
         mock_market_book = mock.Mock()
@@ -162,67 +156,10 @@ class SimulatedMiddlewareTest(unittest.TestCase):
         # defined in the example here: https://github.com/liampauling/flumine/issues/454
         self.assertEqual(mock_order.order_type.liability, 75)
 
-    def test__process_runner_removal_sp_win_inplay(self):
-        order_type = MarketOnCloseOrder(liability=200)
-        mock_order = mock.Mock(
-            selection_id=1234,
-            handicap=0,
-            order_type=order_type,
-            info={},
-            side="LAY",
-            current_order=mock.Mock(size_matched=0),
-            average_price_matched=10,
-        )
-
-        mock_market_book = mock.Mock()
-        mock_market_book.runners = [
-            mock.Mock(selection_id=1234, handicap=0, adjustment_factor=20)
-        ]
-        mock_market = mock.Mock(
-            market_type="WIN", blotter=[mock_order], market_book=mock_market_book
-        )
-        self.middleware._process_runner_removal(mock_market, 12345, 0, 50)
-
-        # The liability of £200 is adjusted by the multiplier of 37.5%, which s
-        # defined in the example here: https://github.com/liampauling/flumine/issues/454
-        self.assertEqual(mock_order.order_type.liability, 75)
-        # Size matched should be 75 / (10.0-1.0) \approx 8.33
-        self.assertEqual(8.33, mock_order.current_order.size_matched)
-
     def test__process_runner_removal_sp_place(self):
         order_type = MarketOnCloseOrder(liability=200)
         mock_order = mock.Mock(
-            selection_id=1234,
-            handicap=0,
-            order_type=order_type,
-            info={},
-            side="LAY",
-            current_order=mock.Mock(size_matched=0),
-            average_price_matched=None,
-        )
-
-        mock_market_book = mock.Mock()
-        mock_market_book.runners = [
-            mock.Mock(selection_id=1234, handicap=0, adjustment_factor=20)
-        ]
-        mock_market = mock.Mock(
-            market_type="PLACE", blotter=[mock_order], market_book=mock_market_book
-        )
-        self.middleware._process_runner_removal(mock_market, 12345, 0, 50)
-
-        # The liability of £200 is reduced by the non runner's adjustment factor of 50%
-        self.assertEqual(mock_order.order_type.liability, 100)
-
-    def test__process_runner_removal_sp_place_inplay(self):
-        order_type = MarketOnCloseOrder(liability=200)
-        mock_order = mock.Mock(
-            selection_id=1234,
-            handicap=0,
-            order_type=order_type,
-            info={},
-            side="LAY",
-            current_order=mock.Mock(size_matched=0),
-            average_price_matched=10.0,
+            selection_id=1234, handicap=0, order_type=order_type, info={}, side="LAY"
         )
 
         mock_market_book = mock.Mock()
@@ -236,8 +173,6 @@ class SimulatedMiddlewareTest(unittest.TestCase):
 
         # The liability of £200 is reduced by the non runner's adjustment factor of 50%
         self.assertEqual(mock_order.order_type.liability, 100)
-        # Size matched should be 100 / (10.0-1.0) \approx 11.11
-        self.assertEqual(11.11, mock_order.current_order.size_matched)
 
     def test__process_streaming_update(self):
         mock_market_book = mock.Mock(
diff --git a/tests/test_orderpackage.py b/tests/test_orderpackage.py
index 339b3d6..730ca3d 100644
--- a/tests/test_orderpackage.py
+++ b/tests/test_orderpackage.py
@@ -61,6 +61,15 @@ class OrderPackageTest(unittest.TestCase):
         self.assertEqual(self.order_package._retry_count, 3)
         mock_time.sleep.assert_called()
 
+    def test_reset_orders(self):
+        mock_order = mock.MagicMock()
+        self.order_package._orders = [mock_order]
+        self.order_package.reset_orders()
+        mock_order.executable.assert_called()
+        mock_order.trade.__enter__.assert_called()
+        self.order_package.reset_orders(True)
+        mock_order.execution_complete.assert_called()
+
     def test_calc_simulated_delay(self):
         config.place_latency = 0.1
         config.cancel_latency = 0.2