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(ws): Add a graceful close mechanism to handle late messages and prevent errors #1129

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

msbrogli
Copy link
Member

@msbrogli msbrogli commented Sep 4, 2024

Motivation

When the stream ended, an ACK message arrived late, causing an error response to be generated.

Check out the state diagram in the Reference-level explanation here: https://github.com/HathorNetwork/internal-rfcs/pull/25

Acceptance Criteria

  1. Do nothing if the ACK received from the client is duplicated.
  2. Stop the streamer if the ACK received from the client decreases.
  3. Stop the streamer if the ACK received from the client is higher than the last sent message sequence number.
  4. Add some protection to the pauseProducing, stopProducing, and resumeProducing methods.
  5. Replace flags _started, _paused, and _stop by a state machine.
  6. Introduce the new state CLOSING which means the streaming has ended and it is just waiting for the last ACK.
  7. Modify streamer to expect ack for the stream end message.

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

@msbrogli msbrogli requested a review from jansegre as a code owner September 4, 2024 18:17
@msbrogli msbrogli requested review from glevco and r4mmer September 4, 2024 18:17
Copy link

github-actions bot commented Sep 4, 2024

🐰Bencher

ReportFri, September 6, 2024 at 17:02:35 UTC
Projecthathor-core
Branchfix/tx-history-streaming-graceful-close
Testbedubuntu-22.04
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Lower Boundary
nanoseconds (ns) | (%)
Latency Upper Boundary
nanoseconds (ns) | (%)
sync-v2 (up to 20000 blocks)✅ (view plot)101,447,530,631.24 (-1.92%)93,094,571,891.75 (91.77%)113,782,254,534.36 (89.16%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

Attention: Patch coverage is 56.62651% with 36 lines in your changes missing coverage. Please review.

Project coverage is 84.42%. Comparing base (ec2563e) to head (22fc4b9).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
hathor/websocket/streamer.py 56.25% 24 Missing and 11 partials ⚠️
hathor/websocket/protocol.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1129      +/-   ##
==========================================
+ Coverage   84.39%   84.42%   +0.03%     
==========================================
  Files         317      317              
  Lines       24240    24291      +51     
  Branches     3684     3695      +11     
==========================================
+ Hits        20457    20508      +51     
+ Misses       3071     3068       -3     
- Partials      712      715       +3     

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

@msbrogli msbrogli force-pushed the fix/tx-history-streaming-graceful-close branch 2 times, most recently from 8936459 to 7e20632 Compare September 5, 2024 18:47
@msbrogli msbrogli self-assigned this Sep 5, 2024
@msbrogli msbrogli force-pushed the fix/tx-history-streaming-graceful-close branch from 7e20632 to f3820a5 Compare September 5, 2024 19:55
jansegre
jansegre previously approved these changes Sep 5, 2024
hathor/websocket/streamer.py Outdated Show resolved Hide resolved
hathor/websocket/streamer.py Show resolved Hide resolved
@msbrogli msbrogli force-pushed the fix/tx-history-streaming-graceful-close branch from f3820a5 to ec2e335 Compare September 6, 2024 15:54
r4mmer
r4mmer previously approved these changes Sep 6, 2024
@msbrogli msbrogli force-pushed the fix/tx-history-streaming-graceful-close branch from ec2e335 to ec33ba9 Compare September 6, 2024 16:03
@msbrogli msbrogli force-pushed the fix/tx-history-streaming-graceful-close branch from ec33ba9 to 93369f3 Compare September 6, 2024 16:13
r4mmer
r4mmer previously approved these changes Sep 6, 2024
@msbrogli msbrogli force-pushed the fix/tx-history-streaming-graceful-close branch from 93369f3 to 8721dc5 Compare September 6, 2024 16:23
@msbrogli msbrogli force-pushed the fix/tx-history-streaming-graceful-close branch 2 times, most recently from 97a7f9c to b528909 Compare September 6, 2024 16:53
@msbrogli msbrogli force-pushed the fix/tx-history-streaming-graceful-close branch from b528909 to 22fc4b9 Compare September 6, 2024 16:59
@msbrogli msbrogli merged commit 22fc4b9 into master Sep 6, 2024
16 checks passed
@msbrogli msbrogli deleted the fix/tx-history-streaming-graceful-close branch September 6, 2024 17:36
@jansegre jansegre mentioned this pull request Oct 4, 2024
2 tasks
@jansegre jansegre mentioned this pull request Dec 11, 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