Skip to content

fix: health endpoint when using --x-asyncio-reactor#940

Merged
luislhl merged 3 commits intomasterfrom
fix/asyncio-reactor-health-endpoint-bug
Feb 8, 2024
Merged

fix: health endpoint when using --x-asyncio-reactor#940
luislhl merged 3 commits intomasterfrom
fix/asyncio-reactor-health-endpoint-bug

Conversation

@luislhl
Copy link
Contributor

@luislhl luislhl commented Feb 6, 2024

Motivation

In case the --x-asyncio-reactor option was used to run hathor-core, we would get the following error when sending requests to /v1a/health:

 Traceback (most recent call last):
File "/usr/local/lib/python3.10/site-packages/twisted/web/server.py", line 227, in process
self.render(resrc)
File "/usr/local/lib/python3.10/site-packages/twisted/web/server.py", line 292, in render
body = resrc.render(self)
File "/usr/local/lib/python3.10/site-packages/twisted/web/resource.py", line 268, in render
return m(request)
File "/usr/local/lib/python3.10/site-packages/hathor/healthcheck/resources/healthcheck.py", line 46, in render_GET
status = asyncio.get_event_loop().run_until_complete(healthcheck.run())
File "/usr/local/lib/python3.10/asyncio/base_events.py", line 625, in run_until_complete
self._check_running()
File "/usr/local/lib/python3.10/asyncio/base_events.py", line 584, in _check_running
raise RuntimeError('This event loop is already running')
RuntimeError: This event loop is already running

The reason for this is that basically Twisted would be running its own asyncio event loop already, so we couldn't run another one.

Acceptance Criteria

  • We should detect whether there is a running event loop and act accordingly. If there is, we just schedule a task on it. If there isn't, we create a new one to run the healthcheck logic.
  • We should now use Deferred's callbacks in this endpoint to handle the response. With them, it becomes easier to adapt to both situations (having a running event loop or not).

Useful References Consulted

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

@luislhl luislhl self-assigned this Feb 6, 2024
@codecov
Copy link

codecov bot commented Feb 7, 2024

Codecov Report

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

Comparison is base (0f5ff70) 85.40% compared to head (0f838e0) 85.29%.

Files Patch % Lines
hathor/healthcheck/resources/healthcheck.py 88.46% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #940      +/-   ##
==========================================
- Coverage   85.40%   85.29%   -0.11%     
==========================================
  Files         290      290              
  Lines       22460    22477      +17     
  Branches     3379     3380       +1     
==========================================
- Hits        19182    19172      -10     
- Misses       2613     2631      +18     
- Partials      665      674       +9     

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

jansegre
jansegre previously approved these changes Feb 7, 2024
@luislhl luislhl force-pushed the fix/asyncio-reactor-health-endpoint-bug branch from cbf7787 to 0f838e0 Compare February 7, 2024 14:18
@luislhl luislhl merged commit 8b13de7 into master Feb 8, 2024
@luislhl luislhl deleted the fix/asyncio-reactor-health-endpoint-bug branch February 8, 2024 13:20
@jansegre jansegre mentioned this pull request Feb 15, 2024
2 tasks
@jansegre jansegre mentioned this pull request Feb 26, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants