-
Notifications
You must be signed in to change notification settings - Fork 1
Deal with timestamp overflows #2
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
base: master
Are you sure you want to change the base?
Conversation
kraklo
left a comment
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.
If I understand this correctly, basically the crashes we were getting was due to format_time trying to format Float::MAX/f64::MAX as that's what's being used as an arbitrary max in every case.
There are also a couple of other places that seem to be checking for Float::MAX, such as
| let latest_arrival_time = if end_time == Float::MAX { |
vrp/vrp-core/src/models/common/domain.rs
Line 81 in e3956e9
| Self { start: 0., end: Float::MAX } |
177 and 202) vrp/vrp-core/src/models/problem/fleet.rs
Line 179 in e3956e9
| end: detail.end.as_ref().and_then(|e| e.time.latest).unwrap_or(Float::MAX), |
vrp/vrp-core/src/models/solution/route.rs
Line 91 in e3956e9
| place: Place { idx: 0, location: 0, duration: 0.0, time: TimeWindow { start: 0.0, end: Float::MAX } }, |
and like a bunch of other places.
Given that the author has used this ubiquitously everywhere, and it's most probably too much effort to try and change it everywhere, I wonder if the only real change that needs to happen is in format_time, given that seems to be the only place where a range error could be encountered.
vrp-pragmatic/src/lib.rs
Outdated
| let time_to_use = TIMESTAMP_MAX.min(time); | ||
| OffsetDateTime::from_unix_timestamp(time_to_use as i64).map(|time| time.format(&Rfc3339).unwrap()).unwrap() |
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.
I wonder if we could just let time handle range errors by letting it return ComponentRange and handling it properly instead of just unwrapping it, using some sort of default MAX_DATETIME or placeholder "Max Date" string.
OffsetDateTime::from_unix_timestamp(time as i64)
.unwrap_or_default(MAX_DATETIME)
.format(&Rfc3339).unwrap()or
let time_result = OffsetDateTime::from_unix_timestamp(time as i64)
match time_result {
Ok(time) => time.format(&Rfc3339).unwrap(),
Err(_) => String::from("Max timestamp"),
}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.
main worry there is that given this is an f64 the sign might be like "max datetime or min datetime" (if we ended up with f64::NEG_INFINITY for example)
not that I think there's any place in the code that does -timestamp but there might be code that does departure - arrival, and then we end up with f64::NEG_INFINITY or something approaching f64::MIN casting out to MAX_DATETIME rather than MIN_DATETIME.
|
Ah! I was searching for Will send in a different set of changes that just goes for the minimal thing |
vrp-core's timestamps go way beyond unix timetsamp range, so here we just clamp.
| // TODO avoid using implicitly unwrap | ||
| OffsetDateTime::from_unix_timestamp(time as i64).map(|time| time.format(&Rfc3339).unwrap()).unwrap() | ||
| // (a priori the above clamping should prevent any potential failure here...) | ||
| let ts = OffsetDateTime::from_unix_timestamp(time).expect("Could not convert value to timestamp"); |
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.
not to get too in the weeds but the error message should be a &'static str and this expect call ... shouldn't really cost much/anythig comp[are to unwrap
| const MAX_TIMESTAMP: i64 = OffsetDateTime::new_in_offset(Date::MAX, Time::MAX, UtcOffset::UTC).unix_timestamp(); | ||
|
|
||
| fn format_time(time: Timestamp) -> String { | ||
| let time: i64 = (time as i64).clamp(MIN_TIMESTAMP, MAX_TIMESTAMP); |
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.
haven't seen anything near MIN_TIMESTAMP but the handling is the same and it reduces the window of failure
|
Could you confirm this works with a sample erroneous payload? |
|
So this minimal fix prevents crashes... in release mode! in dev this crashes The actual scheduling result is the case of breaks does still look nonsensical though. |
I've sent these changes upstream here, along with comments, so might be worth looking over there to see what's up on these changes