introduce cancel to streamer#8025
Conversation
| @@ -83,6 +84,7 @@ pub fn main() { | |||
| let tpu_forward_address = args.tpu_forward_address; | |||
| let max_streams_per_ms = args.max_streams_per_ms; | |||
| let exit = Arc::new(AtomicBool::new(false)); | |||
There was a problem hiding this comment.
I haven't found where exit is set to true. So also don't cancel this token. @lijunwangs am I right that exit is not really used in Vortexor main (only in tests) so I don't need to cancel cancellation token as well? Just to be sure that I haven't missed this.
There was a problem hiding this comment.
Yes. The exit is for testing only. The production vortexor does not have a mechanism to exit gracefully on signals or events currently.
| staked_nodes: Arc<RwLock<StakedNodes>>, | ||
| quic_server_params: QuicServerParams, | ||
| ) -> Result<SpawnServerResult, QuicServerError> { | ||
| let cancel = CancellationToken::new(); |
There was a problem hiding this comment.
This is workaround to preserve the method with the old signature. It is not used anywhere, because we propagate token from the validator.rs since has been created here before.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8025 +/- ##
=========================================
- Coverage 83.0% 83.0% -0.1%
=========================================
Files 826 826
Lines 362199 362298 +99
=========================================
+ Hits 300739 300816 +77
- Misses 61460 61482 +22 🚀 New features to boost your workflow:
|
| cancel.cancel(); | ||
| break; | ||
| } | ||
| sleep(Duration::from_millis(10)).await; |
There was a problem hiding this comment.
This is probably too frequent for such a low frequency event. I would at least 10x it or maybe longer. Or make it configurable for test.
There was a problem hiding this comment.
Agreed, 100-200ms is reasonable here.
There was a problem hiding this comment.
Increased this in the last commit
KirillLykov
changed the title
KirillLykov
force-pushed
the
klykov/introduce-cancel-to-streamer
branch
from
02aeb4b to
5505123
Compare
|
@alexpyattaev this PR has been rebased. I will create a new PR with runtimes creation separately because it happens to big quite big by itself. |
| // handle of the streamer doesn't wait for the child task to finish, so | ||
| // it is not deterministic if the tasks handling connections exit before | ||
| // the assertion below or after. | ||
| sleep(Duration::from_millis(100)).await; |
|
|
||
| // Force connection close by exceeding max_connections_per_peer | ||
| let _pruning_connection = make_client_endpoint(&server_address, None).await; | ||
| // Exit server |
There was a problem hiding this comment.
nice that we can actually exit the thing now =)
alexpyattaev
left a comment
alexpyattaev
left a comment
There was a problem hiding this comment.
Overall this LGTM but wait for @lijunwangs to approve the change to vortexor.
| .fetch_add(1, Ordering::Relaxed); | ||
| } | ||
| stats.total_connections.fetch_sub(1, Ordering::Relaxed); | ||
| debug!("done with stream from {remote_addr}"); |
There was a problem hiding this comment.
--> "Done with connection from ..."
KirillLykov
force-pushed
the
klykov/introduce-cancel-to-streamer
branch
from
9413798 to
17ae0ec
Compare
|
@lijunwangs addressed your comments and rebased with staked/unataked metric change, could you approve if looks good to you? |
4 participants
Problem
When the exit signal is set to true, the streamer's tasks are not exit because not all of them track exit.
Summary of Changes
Use
CancellationTokeninstead of exit fornonblocking::streamer. This is the only way to gracefully close connection tasks.It introduces
spawn_server_with_canceland deprecatesspawn_serverwhich usesexit. So all the tests has been changed to reflect this change.