Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(future.py): add tests and fixes #22101

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

pcriadoperez
Copy link
Contributor

@pcriadoperez pcriadoperez commented Apr 9, 2024

  • Change state lookups to use api instead of _state
  • add tests
  • use reject and resolve function to avoid InvalidStateErrors
  • handle parent cancelation

Existing issues:

  • when closing an exchange if a race future has been resolved and not yet created again with the underlying futures, when closing the exchange you will get a message for Future exception was never retrieved
  • When cancelling a race future we do not propogate the cancel to it's underlying futures as they may be shared with other race futures, this can cause having underling futures without a parent race and end up never retrieved.

The only way to solve this I can think of is we would have refactor to track what futures are part of race_futures and which ones are not, to know which ones to reject and which ones to cancel. And then keep track of the race_futures seperately to throw an ExchangeClosedByUser in the case they all it's promises are cancelled. But this seems a large change that I'm not sure if we want to go into.

Test by running npm run test-python-future
Test by running python3 python/ccxt/pro/test/base/test_close.py

@pcriadoperez pcriadoperez self-assigned this Apr 9, 2024
@pcriadoperez pcriadoperez marked this pull request as draft April 9, 2024 09:23
@pcriadoperez pcriadoperez marked this pull request as ready for review April 10, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants