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

Refactor the code to initialize event loop only from async functions #247

Closed
Tracked by #243
MrNaif2018 opened this issue Oct 10, 2021 · 4 comments · Fixed by #254 or #255
Closed
Tracked by #243

Refactor the code to initialize event loop only from async functions #247

MrNaif2018 opened this issue Oct 10, 2021 · 4 comments · Fixed by #254 or #255
Labels
bug Something isn't working enhancement New feature or request hacktoberfest help wanted Extra attention is needed refactor Something that needs refactor

Comments

@MrNaif2018
Copy link
Member

The time has come. Python 3.10 is released which means that our old ways won't work, and new event loop would be created always.
Recently our tests got broken because of fastapi 0.69.0 release, which uses anyio. We had many issues with too many concurrent operations in progress because of event loop mismatch, and are using some workarounds in pytest-asyncio to make everything use same event loop:
https://github.com/bitcartcc/bitcart/blob/ac129fae929d0853137d327762ef7750537627b6/tests/conftest.py#L32-L34
https://github.com/bitcartcc/bitcart/blob/ac129fae929d0853137d327762ef7750537627b6/tests/conftest.py#L38
https://github.com/bitcartcc/bitcart/blob/ac129fae929d0853137d327762ef7750537627b6/tests/conftest.py#L44

SDK might need to be refactored as well. settings.py should be refactored to a class, most initialization might be moved to other files, possibly Worker class could be introduced to initialize worker.py. Adding new scheduled tasks system could be refactored as well.
redis pool and other connections need to be created on startup from async functions, not on import. Test suite workarounds should be removed and it should just work
If everything works then we can probably remove asyncio.get_event_loop().run_until_complete in favour of asyncio.run

@MrNaif2018 MrNaif2018 added enhancement New feature or request help wanted Extra attention is needed hacktoberfest refactor Something that needs refactor labels Oct 10, 2021
MrNaif2018 added a commit that referenced this issue Oct 10, 2021
MrNaif2018 added a commit that referenced this issue Oct 13, 2021
@MrNaif2018 MrNaif2018 added bug Something isn't working release-blocker labels Oct 13, 2021
@MrNaif2018 MrNaif2018 pinned this issue Oct 13, 2021
@MrNaif2018
Copy link
Member Author

MrNaif2018 commented Oct 13, 2021

Making this a release blocker because when upgrading the packages, uvicorn now uses asyncio.run for each connection, so events look don't match and SDK fails to work because it has saved event loop during module imports.
It was not noticed in tests but only after manual git bisect'ing and reading changelog of all packages
Any help welcome because for now I have no clue on how to do it properly without making developer experience worse

@MrNaif2018
Copy link
Member Author

There are still some issues and fastapi upgrade pending
Regtest tests seems to be failing more frequently than before after #254

@MrNaif2018 MrNaif2018 reopened this Oct 15, 2021
@MrNaif2018 MrNaif2018 linked a pull request Oct 24, 2021 that will close this issue
@MrNaif2018
Copy link
Member Author

This has been implemented. Further testing is required

@MrNaif2018
Copy link
Member Author

This seems to work fine. If some bugs occur after further user testing those can be fixed separately

@MrNaif2018 MrNaif2018 unpinned this issue Nov 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request hacktoberfest help wanted Extra attention is needed refactor Something that needs refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant