Skip to content
This repository has been archived by the owner on Oct 31, 2024. It is now read-only.

feat: terminate stream if client is dropping the connection #463

Merged
merged 10 commits into from
Mar 5, 2024

Conversation

gruberb
Copy link
Contributor

@gruberb gruberb commented Feb 23, 2024

Description

In our current setup, the sequencer is opening a gRPC stream to the tce. If successful, we send delivered_certificates back to the sequencer (or any other client). However, if sequencer shuts down (or restarts), the stream was not closed on the tce side, resulting in multiple open streams with dangling connections.

This PR is fixing this issue by handling the error cases of a streaming message.

Fixes TP-500

Result

024-02-23T15:55:02.245162Z  INFO topos_tce_api::stream: Received an OpenStream command for the stream cbb40b6a-89fc-497a-8594-fa680656bba1
2024-02-23T15:55:02.245189Z  INFO topos_tce_api::runtime: Stream cbb40b6a-89fc-497a-8594-fa680656bba1 is registered as subscriber

... 

2024-02-23T15:55:37.898530Z ERROR topos_tce_api::stream: Stream error: StreamError { stream_id: cbb40b6a-89fc-497a-8594-fa680656bba1, kind: Transport(Unknown) }
2024-02-23T15:55:37.898598Z  INFO topos_tce_api::runtime: Stream cbb40b6a-89fc-497a-8594-fa680656bba1 terminated gracefully

Left ToDo

  • Check if we have to clean up the connection from active_streams
  • Check if we have to clean up any open channels
  • Testing

PR Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added or updated tests that comprehensively prove my change is effective or that my feature works

@gruberb gruberb requested a review from a team as a code owner February 23, 2024 16:02
@gruberb gruberb marked this pull request as draft February 23, 2024 20:40
@gruberb gruberb marked this pull request as ready for review February 26, 2024 14:32
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 70.91%. Comparing base (c41a51a) to head (3c0972f).
Report is 1 commits behind head on main.

Files Patch % Lines
crates/topos-tce-api/src/stream/mod.rs 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #463      +/-   ##
==========================================
- Coverage   71.43%   70.91%   -0.53%     
==========================================
  Files         223      226       +3     
  Lines       12404    12446      +42     
==========================================
- Hits         8861     8826      -35     
- Misses       3543     3620      +77     

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

crates/topos-tce-api/src/stream/mod.rs Outdated Show resolved Hide resolved
crates/topos-tce-api/src/stream/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@Freyskeyd Freyskeyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be good to have tests to cover that a stream close as expected now. I can see that there is two test placeholder that can fit this:

crates/topos-tce-api/src/stream/tests.rs

#[test(tokio::test)]
#[ignore = "not yet implemented"]
async fn closing_client_stream() {}

#[test(tokio::test)]
#[ignore = "not yet implemented"]
async fn closing_server_stream() {}

crates/topos-tce-api/src/stream/mod.rs Outdated Show resolved Hide resolved
@gruberb
Copy link
Contributor Author

gruberb commented Mar 4, 2024

@Freyskeyd can be re-reviewed! Added a test for closing_client_stream. Will add closing_server_stream in TP-887 where we investigate closing the tce side of the stream.

@gruberb gruberb merged commit 2c73f0b into main Mar 5, 2024
21 checks passed
@gruberb gruberb deleted the feat/TP-500 branch March 5, 2024 12:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants