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(p2p): fix update whitelist handler #885

Merged
merged 1 commit into from
Jan 18, 2024
Merged

Conversation

glevco
Copy link
Contributor

@glevco glevco commented Nov 30, 2023

Motivation

There are some Twisted errors that are logged raw, causing each log line to become an alert in OpsGenie, as in this incident: https://github.com/HathorNetwork/on-call-incidents/issues/153.

After some investigation, the reason for this is not 100% clear, but there are some suspicions. This PR attempts to fix at least one case that would cause the problem. We've tested a full node running on mainnet for a few days, and by inspecting its logs, it looks like the error has improved.

Acceptance Criteria

  • Remove custom timeout handling in update_whitelist() to use Deferred.addTimeout().
  • Move the readBody() call to be included in the timeout.
  • Change update_whitelist() so the errback is after the callback, so the errback handles exceptions from the callback.
  • Add unit tests for update_whitelist(), including cases for success, request error, and request timeout.

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 self-assigned this Nov 30, 2023
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

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

Comparison is base (eaf76bd) 85.22% compared to head (baecd84) 85.34%.

Files Patch % Lines
hathor/p2p/manager.py 81.25% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #885      +/-   ##
==========================================
+ Coverage   85.22%   85.34%   +0.11%     
==========================================
  Files         285      285              
  Lines       22368    22362       -6     
  Branches     3371     3369       -2     
==========================================
+ Hits        19063    19084      +21     
+ Misses       2631     2609      -22     
+ Partials      674      669       -5     

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

@glevco glevco force-pushed the fix/update-whitelist branch 3 times, most recently from 80f6e73 to 7c333ea Compare December 7, 2023 22:54
@glevco glevco marked this pull request as ready for review December 7, 2023 23:16
@glevco glevco force-pushed the fix/update-whitelist branch from 7c333ea to 2b048bd Compare January 2, 2024 16:43
hathor/p2p/manager.py Outdated Show resolved Hide resolved
@glevco glevco force-pushed the fix/update-whitelist branch from 2b048bd to a37e5f3 Compare January 15, 2024 19:00
msbrogli
msbrogli previously approved these changes Jan 15, 2024
jansegre
jansegre previously approved these changes Jan 16, 2024
@glevco glevco force-pushed the fix/update-whitelist branch 3 times, most recently from 32a8903 to 192c8b3 Compare January 16, 2024 22:50
@glevco glevco dismissed stale reviews from jansegre and msbrogli via 70ab0d4 January 16, 2024 23:00
@glevco glevco force-pushed the fix/update-whitelist branch from 70ab0d4 to 5150688 Compare January 17, 2024 15:03
@glevco glevco force-pushed the fix/update-whitelist branch from 5150688 to baecd84 Compare January 18, 2024 21:51
@glevco glevco merged commit 8725d21 into master Jan 18, 2024
12 checks passed
@glevco glevco deleted the fix/update-whitelist branch January 18, 2024 23:37
@jansegre jansegre mentioned this pull request Jan 30, 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