Skip to content
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

time: document that a marshaled time does not include location name #28421

Closed
stephens2424 opened this issue Oct 26, 2018 · 3 comments

Comments

Projects
None yet
4 participants
@stephens2424
Copy link

commented Oct 26, 2018

As is, time.Time is not transmittable/storable on its own where it will preserve its location. All of the Marshal/Unmarshal implementations and all the supported string formats encode only the offset or the timezone abbreviation. When parsed or unmarshalled, you often end up with a time.Time in a fixed offset, rather than the original location. In particular, this can lead to unexpected results when performing some operations around Daylight Savings Time because the fixed offset time.Time may or may not observe DST when the original location-laden time.Time would have. Depending on the server location, it's also possible to end up applying DST when it shouldn't apply because of time.Parse's behavior to use time.Local if the local offset matches the parsed timestamp.

As a hallmark example of the problem here is to ask: what month will it be if you add 2840 hours to the reference time used for string formatting? See: https://play.golang.org/p/XQiDRKpKtSa

Overall, this subtlety breaks assumptions that encoded timestamps convey all the information on the original time.Time, and that a decoded time.Time will behave consistently no matter where it's decoded. While a workaround is possible by always separately storing/transmitting the location string in addition to the timestamp, the time package could make that more transparent or at least alert users that the encode/decode may behave unexpectedly with regard to precise timezone rules.

The proposal is to do one or more of the following options. Note that as the author here, my current thinking is option 2 plus option 5 would be the best way forward, but I've listed more possibilities for completeness.

  1. Add the IANA location to what MarshalBinary and UnmarshalBinary do

This will allow gob to losslessly transmit timestamps that will behave the same after encoding/decoding. There are a couple considerations that may rule out this idea:

  • The change might be backward incompatible. I'm not sure if there are Go programs relying on the fact that currently the encoding format results in fixed-offset times after decode.
  • The "improved" parser would still need to handle encoded data from before the change
  • The IANA information might not be useful on systems that don't have tzdata (noting #21881)
  1. Add a Format/Parse directive that includes the IANA location string

This idea is less transparent than the Binary/Gob approach because it requires specifically using a format that includes location. This means it's backward compatible and introduces minimal additional API surface. This possibility still must tackle the problem of how to proceed on systems without available tzdata.

  1. Add an exported struct field to time.Time to toggle encoding behavior

Because the current JSON/text encoding mechanisms for time.Time specify RFC3339, they cannot be changed outright to include the actual location. However, a field could be added to time.Time to alter the encoding behavior. Because the default state of this field could be to encode as the existing format, backward compatibility would be preserved. The decoding routines would need to account for all possible formats and could set that field as necessary.

The two major downsides to this approach I can think of:

  • Additional memory required by time.Time objects
  • That == comparisons would include this field, which seems beyond its intended purpose
  1. Break compatibility in Go2 and include the IANA location in all Marshal/Unmarshal encodings

The big benefit here would be to make time.Time, as an implementation of package encoding's interfaces, to be more true to those interfaces. While package encoding doesn't currently specify whether or not implementations must completely represent the receiver's contents, one might assume they do since generally the process is often treated such that x = decode(encode(x)).

  1. Document this behavior

It's worth warning people that existing encoding methods do not preserve the full location information and that if the program needs to do calendrical operations which may be affected by changes like DST, to transmit/store both the encoded time.Time and its location separately. In particular, since the docs don't specify any format or specifics about the data, one might assume the binary/Gob encoding preserves the full state of the time.Time, when it actually doesn't. While technically this fact is implied for the JSON/text encodings, it's probably a common misconception that the offset in RFC3339 adequately represents location via the offset.

edit: added some formatting

@bradfitz bradfitz changed the title time: proposal: add an encoding or formatting feature to encode and transmit time.Time including location proposal: time: add an encoding or formatting feature to encode and transmit time.Time including location Oct 26, 2018

@gopherbot gopherbot added this to the Proposal milestone Oct 26, 2018

@gopherbot gopherbot added the Proposal label Oct 26, 2018

@andybons

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

After discussion with the proposal committee, we agreed it's best to go with option 5 and document the behavior since you can implement option 2 on your own without any changes to the standard library.

Accepting this proposal to document the behavior only. Thanks.

@andybons andybons changed the title proposal: time: add an encoding or formatting feature to encode and transmit time.Time including location time: document that time.Time does not include location Oct 31, 2018

@andybons andybons modified the milestones: Proposal, Unplanned Oct 31, 2018

@bradfitz bradfitz changed the title time: document that time.Time does not include location time: document that a marshaled time does not include location name Oct 31, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Nov 7, 2018

Change https://golang.org/cl/148117 mentions this issue: time: document that a marshaled time does not include location name

@gopherbot

This comment has been minimized.

Copy link

commented Nov 8, 2018

Change https://golang.org/cl/148339 mentions this issue: time: add a missing comma in the documentation of Time

gopherbot pushed a commit that referenced this issue Nov 8, 2018

time: add a missing comma in the documentation of Time
Updates #28421

Change-Id: I3262c83669bc3cefd2cea6a612e3dc1d4318b2c2
Reviewed-on: https://go-review.googlesource.com/c/148339
Reviewed-by: Rob Pike <r@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.