betcode-org / flumine

flūmine - Betting trading framework
MIT License
164 stars 60 forks source link

EXECUTABLE visible to other threads before processing complete #742

Open foxwoody opened 4 months ago

foxwoody commented 4 months ago

@liampauling as discussed there is a timing issue with replace operations where order.executable() is clearing the update_data fields AFTER setting the order status to executable. This early setting allows other threads to leap in with another replace operation all of which finally results in an exception due to a missing new_price key.

Fix that appears to work is to clear the update_data BEFORE the status turns to executable

    def executable(self) -> None:
        self.update_data.clear()
        self._update_status(OrderStatus.EXECUTABLE)

Also suggest following change to leave setting of any status change as late as possible so all changes are complete before other threads can see a changed status. This would help avoid similar timing issues of other threads jumping the gun before logging and trade adjustments are complete. BTW not checked if trade looks at order status - if it does then needs working around since will see status prior to this transition.

    def _update_status(self, status: OrderStatus) -> None:
        self.status_log.append(status)
        self.complete = self._is_complete()
        if logger.isEnabledFor(logging.INFO):
            logger.info("Order status update: %s" % self.status.value, extra=self.info)
        if self.complete and self.trade.complete and status != OrderStatus.VIOLATION:
            self.trade.complete_trade()
        self.status = status
liampauling commented 4 months ago

Would we not need a thread lock on this function as well?

foxwoody commented 4 months ago

I don't think it is needed on the basis that it is a transition from some state to EXECUTABLE which would normally only be done by the orders thread. As long as the status change is the last thing that happens users wouldn't be expecting to do anything until it shows as being executable - that was the loophole that let me in and broke it - the status was set too early so I went ahead with changing the order.None of this is totally safe from another thread poking values in the order but that is one of the major weaknesses of python. I do have a prototype for a totally protected type of class object such as an order where code takes ownership and nobody can read/write to the object(order) fields or call methods until it is released. That's the long term safest way but it's a fair chunk of work rewriting.