awestlake87 / pyo3-asyncio

Other
300 stars 45 forks source link

Check whether Python Future is already done before setting its result #78

Closed decathorpe closed 1 year ago

decathorpe commented 1 year ago

Python will raise an InvalidStateException if Future.set_value or Future.set_exception are called even though the Future has already completed (or has been cancelled). The code currently checks if the Future has been cancelled before scheduling a call to set_result or set_exception, but if the Future is cancelled from the Python side after this check but before the scheduled set_future / set_exception call is run (i.e. a race condition), Python will raise an InvalidStateError.

This PR introduces checked wrappers for set_result and set_exception which verify that the Future has not been cancelled before calling these methods in a thread-safe way.

Fixes #77

decathorpe commented 1 year ago

PS: I'm not sure that this PR is 100% correct yet. It does prevent the race condition that can lead to InvalidStateErrors, but I'm not sure if checking Future.is_done() is correct in the callback. The existing check only tests for Future.is_cancelled(), and if set_value or set_exception is attempted on a Future that is already done, it probably should raise an InvalidStateError.

And if the check in the callback is changed to is_cancelled, then the existing checks that are done before setting the set_value and set_exception callbacks can probably be dropped (since the check moved to happen "thread-safely" in the callbacks instead).

With these two changes, the code in my PR seems to be more or less equivalent to the changes proposed in #79, so feel free to close / merge / adapt this PR as you see fit. :)

decathorpe commented 1 year ago

I went ahead and implemented the changes I was thinking about.

awestlake87 commented 1 year ago

Merged and released as 0.17 along with a regression test. Also incorporated the canceled check you pushed a few days ago. Let me know if you see any issues downstream!