Skip to content

feat(reactor): implement option to use asyncio reactor #889

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

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

glevco
Copy link
Contributor

@glevco glevco commented Dec 6, 2023

Depends on #888

Motivation

Considering the general improvements we plan to make in the full node related to concurrent and parallel execution, using async/await and multiprocessing, it may be useful to interop with Python's native asyncio. Currently we used Twisted's default reactor, but they provide a reactor that has compatibility with asyncio: AsyncioSelectorReactor. In theory, it is a drop-in replacement for what we're currently using.

This PR adds an experimental CLI option to enable this custom reactor, so we can test its usage in some full nodes for a while. Then, we'll have data to assess whether it's worth it using it as the default, in the future.

Acceptance Criteria

  • Add --x-asyncio-reactor CLI option to enable using the asyncio reactor.
  • Update get_global_reactor() to add support for installing the asyncio reactor.
  • Add reactor type to initial logs.

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@glevco glevco added the refactor label Dec 6, 2023
@glevco glevco self-assigned this Dec 6, 2023
@glevco glevco force-pushed the refactor/reactor/global-reactor branch from 5561862 to d11808f Compare December 7, 2023 15:46
@glevco glevco force-pushed the refactor/reactor/asyncio-reactor branch from 4c33598 to 419f2ee Compare December 7, 2023 15:47
@glevco glevco force-pushed the refactor/reactor/asyncio-reactor branch from 419f2ee to dc9a761 Compare December 7, 2023 16:04
Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (27381c1) 85.35% compared to head (15ba731) 85.34%.
Report is 1 commits behind head on master.

❗ Current head 15ba731 differs from pull request most recent head e84f95d. Consider uploading reports for the commit e84f95d to get more accurate results

Files Patch % Lines
hathor/reactor/reactor.py 63.33% 8 Missing and 3 partials ⚠️
hathor/websocket/factory.py 71.42% 2 Missing ⚠️
hathor/debug_resources.py 66.66% 1 Missing ⚠️
hathor/stratum/stratum.py 80.00% 1 Missing ⚠️
hathor/wallet/resources/thin_wallet/send_tokens.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #889      +/-   ##
==========================================
- Coverage   85.35%   85.34%   -0.01%     
==========================================
  Files         282      284       +2     
  Lines       22280    22443     +163     
  Branches     3366     3396      +30     
==========================================
+ Hits        19018    19155     +137     
- Misses       2592     2604      +12     
- Partials      670      684      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@glevco glevco marked this pull request as ready for review December 7, 2023 16:25
@glevco glevco changed the title refactor(reactor): implement option to use asyncio reactor feat(reactor): implement option to use asyncio reactor Dec 11, 2023
msbrogli
msbrogli previously approved these changes Dec 15, 2023
jansegre
jansegre previously approved these changes Dec 18, 2023
@glevco glevco force-pushed the refactor/reactor/global-reactor branch from 0a39eff to ddb0b61 Compare December 18, 2023 21:39
Base automatically changed from refactor/reactor/global-reactor to master December 18, 2023 22:48
@glevco glevco dismissed stale reviews from jansegre and msbrogli December 18, 2023 22:48

The base branch was changed.

@glevco glevco force-pushed the refactor/reactor/asyncio-reactor branch from 15ba731 to e84f95d Compare December 18, 2023 22:51
@jansegre jansegre merged commit 92c400e into master Dec 19, 2023
@jansegre jansegre deleted the refactor/reactor/asyncio-reactor branch December 19, 2023 17:02
This was referenced Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants