feat(net): adaptable request timeouts #789
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
onbjerg
added
C-enhancement
mattsse
reviewed
Jan 10, 2023
| /// Amount of RTTs before timeout | ||
| const TIMER_MULTIPLIER: u32 = 4; | ||
|
|
||
| // TODO: use a more sophisticated formula |
There was a problem hiding this comment.
worth having a look how geth does this here
There was a problem hiding this comment.
Just checked. They use the same formula.
lambdaclass-user
requested review from
mattsse
and removed request for
Rjected
Codecov Report
@@ Coverage Diff @@
## main #789 +/- ##
==========================================
- Coverage 73.13% 73.10% -0.03%
==========================================
Files 274 275 +1
Lines 28251 28653 +402
==========================================
+ Hits 20660 20946 +286
- Misses 7591 7707 +116
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
mattsse
requested changes
Jan 11, 2023
| @@ -159,31 +172,31 @@ impl ActiveSession { | |||
| on_request!(req, BlockHeaders, GetBlockHeaders); | |||
| } | |||
| EthMessage::BlockHeaders(resp) => { | |||
| on_response!(self, resp, GetBlockHeaders); | |||
| on_response!(resp, GetBlockHeaders); | |||
| req.request.send_err_response(RequestError::Timeout); | ||
| } | ||
| } | ||
|
|
||
| /// Updates the request timeout with a request's timestamps | ||
| fn update_request_timeout(&mut self, sent: Instant, received: Instant) { |
There was a problem hiding this comment.
this makes sense to me,
I'd like to see some sanity tests for this, I think we can split this into to parts:
- standalone,testable function that does the computation
update_request_timeoutcalls this function and updates the time out
There was a problem hiding this comment.
Done! Should we #[inline] the helper function?
and add tests for it
mattsse
approved these changes
Jan 11, 2023
|
It would be useful. You mean something like this? // handle.rs
pub(crate) struct ActiveSessionHandle<'a> {
...
pub(crate) request_timeout: &'a AtomicU64,
}
// active.rs
pub(crate) struct ActiveSession {
...
pub(crate) request_timeout: AtomicU64,
} |
|
yeh |
|
Added! |
mattsse
requested changes
Jan 11, 2023
This reverts last two commits
|
Yeah, sure. Just pushed the changes. Now i'm going to open an issue. |
mattsse
approved these changes
Jan 12, 2023
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #689
This PR adds timeout adapting via RTT estimation for each peer.