Upgrade time to 0.2.5

[kevinpoitra]

Some notable changes:

• The %Z timezone format pattern doesn't exist in time 0.2. Seeing that in C's strptime function, %Z is seemingly ignored (Sun, 07 Nov 1994 08:48:37 PST and Sun, 07 Nov 1994 08:48:37 UTC return the same time), I opted to just remove it from any relevant parsing format strings, and just replaced it with 'GMT' for any format() strings.

• time 0.1's %T format pattern would resolve to %H:%M:%S, whereas time 0.2's %T uses %-H:%M:%S instead (removing any padding). I replaced any instances of %T with %H:%M:%S to keep it the same.

[fafhrd91]

Removing %Z is worrying. I am not sure about that.

[kevinpoitra]

I feel the same way, which is why I submitted this PR to get some feedback on that.

It's strange - in C's format pattern list (which time 0.1 uses verbatim, which changed in 0.2), %Z is documented as being the timezone's name, but the following time 0.1 code returns true, making me think that preserving %Z doesn't matter:

let format = "%a, %d %b %Y %H:%M:%S %Z";

let dt_a = time::strptime("Sun, 07 Nov 1994 08:48:37 PST", format).unwrap();
let dt_b = time::strptime("Sun, 07 Nov 1994 08:48:37 UTC", format).unwrap();

println!("Match: {}", dt_a.eq(&dt_b));

[kevinpoitra]

The Tm returned for both of those strptime calls also matches

Tm {
	tm_sec: 37,
	tm_min: 48,
	tm_hour: 8,
	tm_mday: 7,
	tm_mon: 10,
	tm_year: 94,
	tm_wday: 0,
	tm_yday: 0,
	tm_isdst: 0,
	tm_utcoff: 0,
	tm_nsec: 0
}

Being that those two times are different timezones entirely, you'd expect tm_hour to be different between the two.

[kevinpoitra]

Here's some further reading in regards to %Z's removal in time 0.2, from an issue from 2014 that was updated recently. In general, it seems like %Z would never work without proper IANA TZ support, in either Rust or C. One option is to use UTC offsets instead - %z - but I don't know if that's the right thing to do here.

[fafhrd91]

we should drop chrono as well

[kevinpoitra]

I removed the only instance of chrono from any Cargo.toml here, along with changing any chrono related code, docs, and tests to their time based variants.

Aside from that, even though PrimitiveDateTime is always UTC, I do think that using OffsetDateTime instead is more flexible, so I've gone ahead and switched everything over to that instead.

[kevinpoitra]

One issue I just discovered with that switch to OffsetDateTime: When parsing using OffsetDateTime, being that there's no timezone information within the format pattern we pass to its parse function, it always returns an Err(InsufficientInformation) result (because it expects that UTC offset format pattern, %z, along with a UTC offset within the time string itself), which leads to cookies never having an Expires value (because these parse statements always return Errs).

I've made further adjustments so that we never use OffsetDateTime::parse (because we never deal with UTC offsets). Instead, we always parse time strings with PrimitiveDateTime::parse (and assume that every time string is UTC +0000, seeing as that's what %Z did anyways), then convert it to a OffsetDateTime afterwards.

[fafhrd91]

could you add entries to changes.md to all updated crates

[kevinpoitra]

Done! I also fixed up a few test related errors that popped up from these recent modifications as well.

[kevinpoitra]

After updating my branch with master and having the tests rerun, two tests now fail.

The first one is actix-http::cookie::parse::tests::do_not_panic_on_large_max_ages. Reducing the test down to the following code reproduces the same error:

let max_seconds = Duration::max_value().whole_seconds();
max_seconds + 1;

The reason this is now an issue, but wasn't one before is because time 0.1's Duration::max_value() returned a i64::MAX that represents the amount of milliseconds in the Duration and differs from std::time::Duration::max_value. Because the test then took that max_value and converted it to seconds like this

let max_seconds = Duration::max_value().num_seconds();

max_seconds ended up only being 9223372036854775, and wasn't actually close to overflowing a i64 at all, so adding 1 to it in the test would've never panicked.

If we'd like to keep this test, then I think changing it to something similar to how time itself performs a non-panicking checked_add, like so:

#[test]
fn do_not_panic_on_large_max_ages() {
	let max_duration = Duration::max_value();
	let expected = Cookie::build("foo", "bar").max_age(max_duration.whole_seconds()).finish();
	assert_eq_parse!(format!(" foo=bar; Max-Age={:?}", max_duration.checked_add(Duration::seconds(1)).unwrap_or(max_duration).whole_seconds()), expected);
}

However, this does mean that doing a simple Duration::max_value().whole_seconds() + 1 (or anything that would lead to a similar overflow) would still panic, and we have always have to use whole_seconds as Cookie::max_age takes an i64. Panicking from an overflow during an unchecked addition operation is simply a feature of Rust, so I don't think this is something we should be testing for - the end user should be responsible for ensuring that whatever calculation they're using to create a i64 to pass to max_age doesn't overflow - which makes me think that there's no point in having this test, or that it should be changed to use max_age_time instead like so:

#[test]
fn do_not_panic_on_large_max_ages() {
	let max_duration = Duration::max_value();
	let expected = Cookie::build("foo", "bar").max_age_time(max_duration).finish();
	let overflow_duration = max_duration.checked_add(Duration::seconds(1)).unwrap_or(max_duration);
	assert_eq_parse!(format!(" foo=bar; Max-Age={:?}", overflow_duration.whole_seconds()), expected);
}

[kevinpoitra]

The second one is actix-http::header::shared::httpdate::tests::test_date. It's failing at this assert_eq!, which happens because the String::parse<HttpDate> call ends up using this PrimitiveDateTime::parse format pattern. While that is the correct format pattern to use, instead of outputting the following struct (which is what nov_07 is)

PrimitiveDateTime {
	date: Date { year: 1994, ordinal: 311 },
	time: Time { hour: 8, minute: 48, second: 37, nanosecond: 0 }
}

it outputs this:

PrimitiveDateTime {
	date: Date { year: 94, ordinal: 311 },
	time: Time { hour: 8, minute: 48, second: 37, nanosecond: 0 }
}

which obviously fails due to that year being 94. I'm not sure if this is a bug with time, or if we shouldn't be trying to successfully parse time strings that contain a two digit year.

[kevinpoitra]

For reference in regards to that second test, when using time 0.1 and the old method of parsing (i.e. time::strptime("Sunday, 07-Nov-94 08:48:37 GMT", "%A, %d-%b-%y %T %Z")), the result is this:

Tm {
	tm_sec: 37,
	tm_min: 48,
	tm_hour: 8,
	tm_mday: 7,
	tm_mon: 10,
	tm_year: 94,
	tm_wday: 0,
	tm_yday: 0,
	tm_isdst: 0,
	tm_utcoff: 0,
	tm_nsec: 0
}

which also has a year of 94.

In time 0.1, it seems like %y and %Y would both output a Tm with a two digit year. The assert_eq! above the one that's failing, in the old version of the test (with time 0.1), would also output a year of 94 despite having a four digit year. For example:

time::strptime( "Sun, 07 Nov 1994 08:48:37 GMT", "%a, %d %b %Y %H:%M:%S")

results in the exact same Tm struct as above. So, in time 0.1, every year was always a two digit year, which is not the case in time 0.2, where the year's amount of digits is dependent on how many are in the time string.

Two solutions I've thought of are to either remove support for non-RFC 822 time strings (and only ever try to parse those, rejecting anything else, seeing as they have seemingly invalid year values after being parsed anyways), or modifying the test to perform that failing assert_eq! test with a different, separate nov_07 HttpDate variable that the one already present - one with a year of just 94 (which seems... wrong).

[kevinpoitra]

After some thought and reading through the HTTP RFCs for HTTP Date, I was able to come up with a solution for the test_date failing test issue from above.

• I moved HTTP Date related time parsing into a new file - time_parser.rs. This is used in two places, namely within FromStr in httpdate, and within the Expires header time parsing section of cookie::parse::parse_inner.

• Section 19.3, "Tolerant Applications", of RFC 2616 states that if a time string contains a year that is two digits, to treat it as part of the current century if it's within the next 50 years from the current year, or otherwise treat it as part of the previous century. Using this logic, we convert any successfully parsed RFC 850-based time string's two digit year into a full length one.

[fafhrd91]

i agree with your solution, main problem at the moment is backward compatibility.

[kevinpoitra]

With that fix in place, now the test_date test fails on the last assert_eq!, because it's failing to correctly parse a ANSI C asctime formatted time string.

I've yet to be figure out a solution to this, as every combination of format patterns I've tried hasn't parsed the test's time string successfully.

To show what I mean:

time 0.1, what it was before any of these changes:

time::strptime("Sun Nov  7 08:48:37 1994", "%c")

produced this Tm struct:

Tm {
	tm_sec: 37,
	tm_min: 48,
	tm_hour: 8,
	tm_mday: 7,
	tm_mon: 10,
	tm_year: 94,
	tm_wday: 0,
	tm_yday: 0,
	tm_isdst: 0,
	tm_utcoff: 0,
	tm_nsec: 0
}

%c changed in time 0.2. For example, the following time 0.2 code:

PrimitiveDateTime::parse("Sun Nov  7 08:48:37 1994", "%c")

produces an Err(InvalidDayOfMonth) error.

I've been attempting to recreate time 0.1's %c format pattern functionality, but with little success.

Using pattern %a %b %_d %H:%M:%S %Y (utilizing the underscore to add a space padding in front of the day) produces a Err(UnexpectedCharacter { expected: ' ', actual: '0' }). Changing the time string to one that has a two-digit day (for example: Sun Nov 17 08:48:37 1994) results in a successfully parsed PrimitiveDateTime struct of

PrimitiveDateTime {
	date: Date { year: 1994, ordinal: 320 },
	time: Time { hour: 8, minute: 48, second: 37, nanosecond: 0 }
}

This seems like a bug within time itself. I'll investigate further and open an issue within time's repo if that's the case.

[jhpratt]

@fafhrd91 Time v0.1's support for %Z was basically nonexistent anyways, only supporting "UTC" and "GMT" if I remember correctly. It should be preferred to use that explicitly, at least until time gets tzdb support.

And just looking through this issue, Duration::max_value() is actually the maximum value now. Adding any amount (even one nanosecond) is guaranteed to result in an overflow.

[jhpratt]

PSA: I may be reverting the change to a non-additive "alloc" feature in favor of the additive "std" feature. I thought the change was necessary due to serde — turns out it's not.

Sorry for the back-and-forth!

[jhpratt]

PR is up from mine to the fork. All that's changed is the Cargo.tomls, as things are otherwise identical.

I assure you you can merge this now 😛 v0.2.3 has been yanked as well, so anything backwards-incompatible has been extremely limited.

[kevinpoitra]

Merged @jhpratt's PR into my fork - this should be ready to merge now!

[kevinpoitra]

@jhpratt: Just now noticing some issues with running cargo check after your change to 0.2.4 - it doesn't seem to want to find offset!, and is confused about what type time::parse returns.

[jhpratt]

For offset!, I presume it just needs to be imported into scope. I'm on my phone now, so I can't see the checks. I can take a look on my laptop later.

With regard to parsing, you can either use the specific type (like OffsetDateTime::parse) or turbofish (time::parse::<OffsetDateTime>) to provide the compiler the necessary knowledge. Not sure why that regressed, though.

[kevinpoitra]

The missing format! macro ended up just being a missing use statement.

As for the type confusion though, I brought it up because using PrimitiveDateTime::parse or time::parse::<PrimitiveDateTime> seem counter intuitive when time::parse should figure out what type the program expects. I did a bit of digging, here's what I found:

Using this time::parse call within one of actix-http's tests as an example, within a Rust project that only has time 0.2.4 as a dependency and one print_time function which takes an OffsetDateTime (similar to actix-http's set_expires function), I get the following output:

Starting off with how the code is structured in the test:

let time_str = "Wed, 21 Oct 2015 07:28:00 GMT";
let time = time::parse(time_str, "%a, %d %b %Y %H:%M:%S").unwrap().using_offset(offset!(UTC));
print_time(time);

Output:

error[E0282]: type annotations needed
 --> src\main.rs:7:63
  |
7 |     let time = time::parse(time_str, "%a, %d %b %Y %H:%M:%S").unwrap().using_offset(offset!(UTC));
  |                -----------------------------------------------^^^^^^--
  |                |                                              |
  |                |                                              cannot infer type for type parameter `T`
  |                this method call resolves to `T`
  |
  = note: type must be known at this point

Separating the offset into its own variable:

let time_str = "Wed, 21 Oct 2015 07:28:00 GMT";
let time = time::parse(time_str, "%a, %d %b %Y %H:%M:%S").unwrap();
let offset = time.using_offset(offset!(UTC));
print_time(offset);

Output:

error[E0282]: type annotations needed
 --> src\main.rs:8:18
  |
7 |     let time = time::parse(time_str, "%a, %d %b %Y %H:%M:%S").unwrap();
  |         ---- consider giving `time` a type
8 |     let offset = time.using_offset(offset!(UTC));
  |                  ^^^^ cannot infer type
  |
  = note: type must be known at this point

Lastly, removing the offset completely:

let time_str = "Wed, 21 Oct 2015 07:28:00 GMT";
let time = time::parse(time_str, "%a, %d %b %Y %H:%M:%S").unwrap();
print_time(time);

Output:
Because we aren't passing that time variable to using_offset, Rust now sees that it should be given the OffsetDateTime type due to print_time's argument type. However, at runtime, the program panics with the following error, I assume because we're trying to create a OffsetDateTime from a time format that has no offset:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: InsufficientInformation',

Changing print_time to take a PrimitiveDateTime instead makes the program run successfully, but we probably don't want to change set_expires to take PrimitiveDateTime.

I think the best thing to do here is to use PrimitiveDateTime::parse in the sections where time::parse would be immediately followed by an using_offset call, seeing as we know we're always working with some date and time format and we'll always not care about what offset the time is in (as the tests should never have a UTC offset), but if you think of anything better, I'd be happy to change it.

[JohnTitor]

Okay, I just got the overview.
Overall looks good with some nits:

1. Update the mentioned version to 0.2.4 (or even 0.2.5 if you want) in the changelogs since 0.2.3 is yanked.
2. Update the changelog of the test-server crate also.

And I think this can be considered as a breaking change in some crate, so I'm going to mark so.

[JohnTitor]

Thanks @kevinpoitra and @jhpratt!

[kevinpoitra]

You're welcome! As soon as the actix, actix-web, actix-http, and actix-service crates have their next versions published, I'll go ahead and finish up this related actix-redis PR.