Make Sniffer unblocking and upgrade TP#1592
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1592 +/- ##
==========================================
- Coverage 21.92% 21.14% -0.79%
==========================================
Files 135 135
Lines 9573 9964 +391
==========================================
+ Hits 2099 2107 +8
- Misses 7474 7857 +383 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
This is ready for another review |
| ); | ||
| }); | ||
|
|
||
| let receiver_outgoing_cloned = receiver_outgoing.clone(); | ||
| task::spawn(async move { | ||
| select!( | ||
| _ = tokio::signal::ctrl_c() => { }, |
There was a problem hiding this comment.
With this commit, the lifetimes of multiplexed streams are now tied together—if one stream drops, the select block drops the rest. Since network helpers are just a layer that adds encryption and multiplexing, this feels a bit counterintuitive. I think the previous commit would’ve worked fine too.
That said, even if it seems counterintuitive, it raises an interesting point. In SV2, we know that every entity will always use both the read and write streams, not just one. Streams will only drop when the app shuts down. Based on past experience with network helpers, one stream usually drops while the other blocks the executor. So if we go with this select setup, we might not even need ctrl_c signaling anymore.
Either way, we should be clear on what we want from network helpers before moving forward. I would like to hear other team members opinions on this.
There was a problem hiding this comment.
I dont get how the ctrl_c is related to this and what are you suggesting here?
.. loop instead of panicing if upstream is yet to come up
|
Removed last commit. This should be good to go. |
resolves #1591 and #1602