-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Protocol] Retry failed requests #95
Changes from 7 commits
b129142
7dcf0f0
5ed2992
2ac0a64
2127cae
a81a9bc
88cdcfd
5bc118d
cb03bc9
4d54cc2
102d3b8
ecf0f2c
a76f6fa
2d972c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ use crate::buffer::{Buffer, Utf8Decoder}; | |
use crate::protocols::quake::types::Response; | ||
use crate::protocols::types::TimeoutSettings; | ||
use crate::socket::{Socket, UdpSocket}; | ||
use crate::utils::retry_on_timeout; | ||
use crate::GDErrorKind::{PacketBad, TypeParse}; | ||
use crate::{GDErrorKind, GDResult}; | ||
use std::collections::HashMap; | ||
|
@@ -95,7 +96,10 @@ pub fn client_query<Client: QuakeClient>( | |
address: &SocketAddr, | ||
timeout_settings: Option<TimeoutSettings>, | ||
) -> GDResult<Response<Client::Player>> { | ||
let data = get_data::<Client>(address, timeout_settings)?; | ||
let retry_count = TimeoutSettings::get_retries_or_default(&timeout_settings); | ||
let data = retry_on_timeout(retry_count, || { | ||
get_data::<Client>(address, timeout_settings.clone()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Undesirable clone (see before) |
||
})?; | ||
let mut bufferer = Buffer::<LittleEndian>::new(&data); | ||
|
||
let mut server_vars = get_server_values(&mut bufferer)?; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -146,12 +146,19 @@ pub struct CommonPlayerJson<'a> { | |
pub struct TimeoutSettings { | ||
read: Option<Duration>, | ||
write: Option<Duration>, | ||
retries: usize, | ||
} | ||
|
||
impl TimeoutSettings { | ||
/// Construct new settings, passing None will block indefinitely. | ||
/// Passing zero Duration throws GDErrorKind::[InvalidInput]. | ||
pub fn new(read: Option<Duration>, write: Option<Duration>) -> GDResult<Self> { | ||
/// | ||
/// The retry count is the number of extra tries once the original request | ||
/// fails, so a value of "0" will only make a single request, whereas | ||
/// "1" will try the request again once if it fails. | ||
/// The retry count is per-request so for multi-request queries (valve) if a | ||
/// single part fails that part can be retried up to `retries` times. | ||
pub fn new(read: Option<Duration>, write: Option<Duration>, retries: Option<usize>) -> GDResult<Self> { | ||
if let Some(read_duration) = read { | ||
if read_duration == Duration::new(0, 0) { | ||
return Err(InvalidInput.context("Read duration must not be 0")); | ||
|
@@ -164,22 +171,39 @@ impl TimeoutSettings { | |
} | ||
} | ||
|
||
Ok(Self { read, write }) | ||
Ok(Self { | ||
read, | ||
write, | ||
retries: retries.unwrap_or(0), | ||
}) | ||
} | ||
|
||
/// Get the read timeout. | ||
pub const fn get_read(&self) -> Option<Duration> { self.read } | ||
|
||
/// Get the write timeout. | ||
pub const fn get_write(&self) -> Option<Duration> { self.write } | ||
|
||
/// Get number of retries | ||
pub const fn get_retries(&self) -> usize { self.retries } | ||
|
||
/// Get the number of retries if there are timeout settings else fall back | ||
/// to the default | ||
pub fn get_retries_or_default(timeout_settings: &Option<TimeoutSettings>) -> usize { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This maybe shouldn't be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think thats fine for now. |
||
timeout_settings | ||
.as_ref() | ||
.map(|t| t.get_retries()) | ||
.unwrap_or_else(|| TimeoutSettings::default().get_retries()) | ||
} | ||
} | ||
|
||
impl Default for TimeoutSettings { | ||
/// Default values are 4 seconds for both read and write. | ||
/// Default values are 4 seconds for both read and write, no retries. | ||
fn default() -> Self { | ||
Self { | ||
read: Some(Duration::from_secs(4)), | ||
write: Some(Duration::from_secs(4)), | ||
retries: 0, | ||
} | ||
} | ||
} | ||
|
@@ -273,7 +297,7 @@ mod tests { | |
let write_duration = Duration::from_secs(2); | ||
|
||
// Create new TimeoutSettings with the valid durations | ||
let timeout_settings = TimeoutSettings::new(Some(read_duration), Some(write_duration))?; | ||
let timeout_settings = TimeoutSettings::new(Some(read_duration), Some(write_duration), None)?; | ||
|
||
// Verify that the get_read and get_write methods return the expected values | ||
assert_eq!(timeout_settings.get_read(), Some(read_duration)); | ||
|
@@ -291,7 +315,7 @@ mod tests { | |
|
||
// Try to create new TimeoutSettings with the zero read duration (this should | ||
// fail) | ||
let result = TimeoutSettings::new(Some(read_duration), Some(write_duration)); | ||
let result = TimeoutSettings::new(Some(read_duration), Some(write_duration), None); | ||
|
||
// Verify that the function returned an error and that the error type is | ||
// InvalidInput | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clone here is undesirable but needed because there is no struct to take ownership of it here.