Test for errors during large message send/receipt + retry on send fail #289
Conversation
joshuef
force-pushed
the
Tests-TimedOuts
branch
3 times, most recently
from
6f0d8bb
to
b322289
Compare
joshuef
force-pushed
the
Tests-TimedOuts
branch
6 times, most recently
from
829b216
to
0ffb369
Compare
joshuef
changed the title
Return quinn error from deduplicator remove an unneeded clone reduce max attempts
| @@ -3,3 +3,4 @@ | |||
| Cargo.lock | |||
| *.*~ | |||
| .idea | |||
| *.log | |||
There was a problem hiding this comment.
I mean it needs an extra line break at the end of file.
otherwise it will show this red icon.
| @@ -193,15 +201,22 @@ pub(super) fn listen_for_incoming_messages( | |||
| ) { | |||
| let src = *remover.remote_addr(); | |||
| let _ = tokio::spawn(async move { | |||
| debug!("qp2p another incoming listerner spawned"); | |||
| let _ = future::join( | |||
| match future::try_join( | |||
There was a problem hiding this comment.
just to confirm.
so this try_join will return on the first error encounter.
which I am not sure what's happening to the other.
Is the read on the other stream will be terminated at the same time?
There was a problem hiding this comment.
Yeh the other future will be cancelled as I understand.
| error!( | ||
| "Failed reading from a bi-stream for peer {:?} with error: {:?}", | ||
| warn!( | ||
| "Failed reading from a bi-stream for peer {:?} with: {:?}", | ||
| peer_addr, err | ||
| ); | ||
| break; |
There was a problem hiding this comment.
hmm... no sure about breaking here (and that error of FinishedEarly), which actually now returns Ok from the function.
So we don't need (or don't know what to do) under these error situations?
There was a problem hiding this comment.
As I can figure, it's a question of "can this connection still be used". If so, we do not error.
There was a problem hiding this comment.
So we were breaking before here, so that's not a change.
As I understand, here we've received the bytes and it's the processing that fails (some message fails), but the connection is still valid, so we don't want to err out and cause a disconnect.
| let mut attempts: usize = 0; | ||
| let mut res = self.try_send_message(msg.clone(), dest).await; | ||
|
|
||
| while attempts < MAX_ATTEMPTS && res.is_err() { |
There was a problem hiding this comment.
we will try resends for 10 times, then on disconnection, try re-connect for 10 times as well ?
and each takes an interval of 0.5s, which makes a total of 50s to detect a really failed connection ?
There was a problem hiding this comment.
Could be. (worth nothing, this has already been updated to MAX_ATTEMPTS=5 ).
What's an acceptable time here? How many tries do we want?
50s doesn't seem too bad. to be sure, vs just a 30s timeout eg
There was a problem hiding this comment.
mainly just confirming this setup is ok.
| @@ -28,6 +28,17 @@ pub(crate) struct Connection { | |||
| remover: ConnectionRemover, | |||
| } | |||
|
|
|||
| /// Disconnection events, and the result that led to disconnection. | |||
Close issue maidsafe/safe_network#69