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(sync-v2): stop _process_transaction on error #877

Merged
merged 1 commit into from
Nov 24, 2023

Conversation

jansegre
Copy link
Member

@jansegre jansegre commented Nov 20, 2023

Motivation

During the last QA when a node was syncing from scratch, we caught a case where more than one unexpected transaction was sent on a streamer, the first one correctly failed the streamer client, but the second one lead to an error because the deferred had already been called.

Acceptance Criteria

  • Add test to reproduce the case;
  • Make it so TransactionStreamingClient.fails will not call self._deferred.errback if the deferred has already been called.

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

@jansegre jansegre requested review from msbrogli and glevco November 20, 2023 19:39
@jansegre jansegre self-assigned this Nov 20, 2023
@jansegre jansegre mentioned this pull request Nov 20, 2023
2 tasks
@jansegre jansegre changed the base branch from master to feat/trigger-sendline-regex November 21, 2023 22:49
@jansegre jansegre force-pushed the fix/stop-process-tx-on-error branch 2 times, most recently from aff25ed to 39e5169 Compare November 22, 2023 16:43
@jansegre jansegre marked this pull request as ready for review November 22, 2023 16:43
@jansegre jansegre force-pushed the fix/stop-process-tx-on-error branch from 39e5169 to 46f0041 Compare November 22, 2023 16:48
@jansegre jansegre force-pushed the fix/stop-process-tx-on-error branch from 46f0041 to f65fdb4 Compare November 23, 2023 17:23
Copy link

codecov bot commented Nov 23, 2023

Codecov Report

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

Comparison is base (2a86be0) 85.30% compared to head (f65fdb4) 85.28%.
Report is 2 commits behind head on master.

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

Files Patch % Lines
hathor/p2p/sync_v2/transaction_streaming_client.py 40.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #877      +/-   ##
==========================================
- Coverage   85.30%   85.28%   -0.02%     
==========================================
  Files         281      281              
  Lines       22364    22385      +21     
  Branches     3388     3392       +4     
==========================================
+ Hits        19077    19091      +14     
- Misses       2603     2609       +6     
- Partials      684      685       +1     

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

msbrogli
msbrogli previously approved these changes Nov 24, 2023
@msbrogli msbrogli force-pushed the feat/trigger-sendline-regex branch 2 times, most recently from 8a6abb5 to 7b6eb34 Compare November 24, 2023 15:11
Base automatically changed from feat/trigger-sendline-regex to master November 24, 2023 15:19
@msbrogli msbrogli dismissed their stale review November 24, 2023 15:19

The base branch was changed.

@jansegre jansegre force-pushed the fix/stop-process-tx-on-error branch from f65fdb4 to ffd1cd1 Compare November 24, 2023 15:27
@jansegre jansegre merged commit 965fba3 into master Nov 24, 2023
8 checks passed
@jansegre jansegre deleted the fix/stop-process-tx-on-error branch November 24, 2023 15:48
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
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants