8 ms·

A nasty bit of undefined timezone behavior in Golang

95 points by fanf2 5y ago

SAI_Peregrinus 5y ago
Unexpected for go, but very much like what happens with C locales when they change.
- ilammy 5y ago
  Every once in a while I open this commit [1] and read it, just to remind myself to try and avoid making stupid decisions within my power, hopefully preventing my APIs from becoming a standardized mess that C locales are. [1]: https://github.com/mpv-player/mpv/commit/1e70e82baa9193f6f027338b0fab0f5078971fbe https://github.com/mpv-player/mpv/commit/1e70e82baa9193f6f02...
  - pronoiac 5y ago
    > But locales give a delicious sweet spot of brokenness, where things are broken enough to cause neverending pain, but not broken enough that enough effort would have spent to fix it completely. Oh, this feels familiar. I most recently had this feeling when reading about the git staging area, where that term is common everywhere but the man pages.
purplerabbit 5y ago
Really loved the voice of this article. Very funny. Just wrapping up a read of “the go programming language”, and this article called out literally every aspect of go I was surprised/annoyed to discover. The validation is much appreciated
- silisili 5y ago
  Just for reference, complete opposite for me. I found it rambling and missing the mark. I expected a simple repro case exposing the bug. I got many paragraphs of random relevance and still not positive what the bug actually is. Even a link to an issue would have been helpful.
  - blowski 5y ago
    And the memes. A couple of particularly relevant and funny memes is ok, but this article had a lot that were neither.
pphysch 5y ago
Storing timezone info by reference, and having dynamic timezones (like Local) are both great features for dealing with the messiness of time. But there are always tradeoffs...
marcan_42 5y ago
This isn't so much undefined behavior as unexpected statefulness, but yeah, it shouldn't have been designed like this. I've had a couple of run-ins with the Golang runtime myself, which were actual bugs. The first one was an issue with C interop triggered by my first production Go app; it had been reported but never successfully debugged, so I did so. It took a couple evenings to get it figured out, and a then a month to get the fix merged upstream. To be honest, that put me off of using Go for anything serious; I just never got the feeling that it's a solidly designed language and runtime. The second one I ran into as a user, and I think has ended up on HN a few times: https://marcan.st/2017/12/debugging-an-evil-go-runtime-bug/ https://marcan.st/2017/12/debugging-an-evil-go-runtime-bug/
- barsonme 5y ago
  Frankly, you did some amazing debugging. > and a then a month to get the fix merged upstream. To be honest, that put me off of using Go for anything serious; I just never got the feeling that it's a solidly designed language and runtime. This seems a little picky, though. Ian mailed the CL one day after you found the root cause. The CL was merged a couple days later. The fix was added to the very next patch release. That's a good turnaround time for a bug, especially a vDSO corner case. Go has its fair share of (sometimes quite annoying) oddities. In hindsight, some of the design and implementation choices were definitely incorrect. But they were still well thought out and I can't say I'd have made a better argument or decision at that point in time, either.
  - marcan_42 5y ago
    That was the second bug. The first bug was this one (CL sent on 2014-08-19 and merged on 2014-09-24): https://github.com/golang/go/issues/7978 https://github.com/golang/go/issues/7978 https://codereview.appspot.com/131910043 https://codereview.appspot.com/131910043 I particularly like this comment: "Ping. Could somebody review this? Internal Google users suspect this is the cause of some of their problems." I guess once fixing Google production, always fixing Google production (I ran into this at my next job after leaving Google). But after that first bug, and having to dive deep into the golang runtime... it just didn't look well engineered to me, at all. Things like having architecture-specific details scattered open-coded all through stuff like the stack tracer (IIRC when they added ARM support they had to add an `lr` argument to a ton of codepaths). That first app was doing some Cgo stuff, and after looking carefully through how Cgo is supposed to work and how it actually works and what guarantees there are... it was just a mess. The only way to guarantee memory wouldn't be yanked out from under C was, apparently, to use malloc() and copy, which means you cannot do zero-copy calls into C code. There was no formal specification for what memory pinning is guaranteed or not. This is a general theme in Go. It feels like an ad-hoc language with a bunch of interesting design decisions, but largely superficial; in the end the deeper you dive into it, the more the answer to how things work is "shrug" (and "may change in the future"). It's clearly written by C people with a C mindset, trying to make a "better C" without letting go of the bad habits that C brings with it, and this is more evident the deeper you look. And I say this as primarily a C and Python coder. Then there's the whole "reinventing libc" insanity, even on macOS (where no such ABI stability was guaranteed, but they did it anyway, and that ended up with a macOS update breaking all Go apps). On Windows they can't get away with that, so they use Cgo instead, and then we're back at the Cgo mess/overhead. This design decision is also, ultimately, how the vDSO problem happened. I've also seen a tendency towards bloat in Go (see: the stories about Go binary size forever increasing). I find it particularly crazy that these days they need to have metadata that describes stack layout at every possible instruction in the program, to make their GC work. I don't mean all of this in a "Go is a bad language that should go away" sense; it has things it does well, and if I ever have to put together a high-performance concurrent network server with no external dependencies I might choose Go. No language is perfect. I just find that, after ending up deep in the bowels of Go, I'm not really inclined to default to it for anything that isn't very clearly its forte. I'm really looking forward to learning Rust, which seems much more serious in this regard, but I had a very different problem there; after writing a relatively simple Rust app once, I tried to engineer a more complex application in it, and ended up unable to wrap my head around how to make an abstract interface work with proper ownership rules. I should go back to doing something in Rust some day, something a bit less ambitious...
tapirl 5y ago
> Reads from a closed channel block, sends on a closed channel panic "never block"?
- wahern 5y ago
  Maybe they confused closed and nil channels. Reads from a nil channel block, as do sends.
zimpenfish 5y ago
Oh, this might explain the rare[1] local-only[2] "m=+0.00blah" test fails on deep `time.Time` comparisons we sometimes get talking to Firebase. (We don't actually use `time.Local` anywhere but I'm guessing one of the support libraries does.) [1] Once a month, maybe [2] But never on the CI - only when running the tests on developer machines
xyzzy_plugh 5y ago
This does not appear to be undefined at all. The issue is using reflect.DeepEqual to compare structs with unexported fields. The fields are an implementation detail, and comparing time.Time values is generally incorrect. Here are the actual docs from https://pkg.go.dev/time#Time.Equal https://pkg.go.dev/time#Time.Equal > Equal reports whether t and u represent the same time instant. Two times can be equal even if they are in different locations. For example, 6:00 +0200 and 4:00 UTC are Equal. See the documentation on the Time type for the pitfalls of using == with Time values; most code should use Equal instead. and > Time instants can be compared using the Before, After, and Equal methods. The Sub method subtracts two instants, producing a Duration. The Add method adds a Time and a Duration, producing a Time. and > Note that the Go == operator compares not just the time instant but also the Location and the monotonic clock reading. Therefore, Time values should not be used as map or database keys without first guaranteeing that the identical Location has been set for all values, which can be achieved through use of the UTC or Local method, and that the monotonic clock reading has been stripped by setting t = t.Round(0). In general, prefer t.Equal(u) to t == u, since t.Equal uses the most accurate comparison available and correctly handles the case when only one of its arguments has a monotonic clock reading. If I were reviewing this code, the bug squarely lands on the shoulders of the author who chose to use reflect.DeepEqual (or similar function available in many test helper libraries) here. Frankly, I find code that uses such blind comparisons to be lazy and prone to bugs exactly like this one. For readers unfamiliar with Go, this pattern is not unique to the time library. In general you avoid these sort of comparisons, at least without reading the docs. It's one thing if it's your own type or a fully-exported type or something obvious but this is just so clearly wrong. Furthermore I find this entire blog in pretty bad taste. Read the docs, own up to your own mistake, don't make blind assumptions about data types, don't lean on reflection to be lazy. Trying to throw shade at the Go authors crosses the line for me here, and name-calling Rob Pike is ultra immature. Trying to be cute, I guess? But ultimately I now think poorly of this business and its engineers. Bad form, folks.
- cinntaile 5y ago
  > Trying to throw shade at the Go authors crosses the line for me here, and name-calling Rob Pike is ultra immature. The whole blogpost is written in a tongue in cheek fashion, I don't think it's meant as name-calling.
hajhatten 5y ago
Why not just explicitly use UTC everywhere?
- solidsnack9000 5y ago
  Yeah, reading this article, it seemed like using a wildcard time zone was the real bug.
sgarman 5y ago
I feel bad for that new guy. I hope that was not the actual conversation.
- papaf 5y ago
  One thing I would recommend when someone blames a bug on you -- without hesitation, always say "yes, I will take a look at it". This way if you have to push back and say it wasn't you, you have much more impact because you have investigated the problem. Edit: but I agree, that is no way to treat a colleague.
- exciteabletom 5y ago
  >I demonstrated outstanding leadership and blamed him without any further evidence. At least the Author seems to know it wasn't OK.
- sqldba 5y ago
  Thanks! I felt the same way. Glad I’m not the only one. I’ve had people attack me like that before and it’s not great. It’s always someone in a position of power, too.
  - shock-value 5y ago
    It’s embarrassing and shameful behavior by the author, and even more embarrassing that he was willing to share it publicly. If it’s the culture of that startup that this kind of stuff is just typical day-to-day employee interaction, I don’t know who would want to work there, especially given how hot the job market is now.
barsonme 5y ago
The problem here isn't with time.Time, it's with reflect.DeepEqual. Using it in your code to compare arbitrary data is almost always incorrect. And IMO, adding it to the stdlib was a mistake. time.Time values should only be compared for equality using the Equal method (or IsZero), but reflect.DeepEqual does not know that. This can happen with any type, not just time.Time. For example, I have a decimal library. It can represent 200 as either (mantissa=2, exponent=2) or (mantissa=200, exponent=0). Comparing those two numbers for equality will return true, but comparing them with reflect.DeepEqual will return false. So, it's not a "nasty bit of undefined timezone behavior." It's somebody misusing an API.
- tialaramex 5y ago
  This feels to me like maybe a good example for operator overloading. Often the "good" case of operator overloading is given as some fancy numeric type, like Complex or Matrix or something, for which some arithmetic makes sense but the fundamental integer and floating point operators don't cut it. But here is a case where programmer defined overloading of equality shows a benefit, if each type (including time.Time) takes responsibility for knowing how to compare that type then nobody else needs to know this detail and the natural comparison operators work, rather than needing to awkwardly define == and Equal separately and then forever live with people choosing the wrong one by mistake. After all, in the absence of such an overload (and I agree having it would interfere with Go's goal of simplicity) this would likely happen even without reflect.DeepEqual because people want to do this, and "You can't do this safely" is unlikely to have better results in Go than other languages.
  - formerly_proven 5y ago
    This excerpt from the Go docs is an excellent case for (some) operating overloading: > Note that the Go == operator compares not just the time instant but also the Location and the monotonic clock reading. Therefore, Time values should not be used as map or database keys without first guaranteeing that the identical Location has been set for all values, which can be achieved through use of the UTC or Local method, and that the monotonic clock reading has been stripped by setting t = t.Round(0). In general, prefer t.Equal(u) to t == u, since t.Equal uses the most accurate comparison available and correctly handles the case when only one of its arguments has a monotonic clock reading.
jbaviat 5y ago
Je suis en train de me faire un devis pour
wereHamster 5y ago
Why is implicit global state still a thing in modern languages?
dblooman 5y ago
That message exchange screenshot is unfortunately seen too often in companies.
j16sdiz 5y ago
If you ask me, the problem lies in SQL datetime (without timezone) data type, not golang.

95 points by fanf2 5y ago

SAI_Peregrinus 5y ago

Unexpected for go, but very much like what happens with C locales when they change.

ilammy 5y ago
Every once in a while I open this commit [1] and read it, just to remind myself to try and avoid making stupid decisions within my power, hopefully preventing my APIs from becoming a standardized mess that C locales are. [1]: https://github.com/mpv-player/mpv/commit/1e70e82baa9193f6f027338b0fab0f5078971fbe https://github.com/mpv-player/mpv/commit/1e70e82baa9193f6f02...
- pronoiac 5y ago
  > But locales give a delicious sweet spot of brokenness, where things are broken enough to cause neverending pain, but not broken enough that enough effort would have spent to fix it completely. Oh, this feels familiar. I most recently had this feeling when reading about the git staging area, where that term is common everywhere but the man pages.

purplerabbit 5y ago

Really loved the voice of this article. Very funny. Just wrapping up a read of “the go programming language”, and this article called out literally every aspect of go I was surprised/annoyed to discover. The validation is much appreciated

silisili 5y ago
Just for reference, complete opposite for me. I found it rambling and missing the mark. I expected a simple repro case exposing the bug. I got many paragraphs of random relevance and still not positive what the bug actually is. Even a link to an issue would have been helpful.
- blowski 5y ago
  And the memes. A couple of particularly relevant and funny memes is ok, but this article had a lot that were neither.

pphysch 5y ago

Storing timezone info by reference, and having dynamic timezones (like Local) are both great features for dealing with the messiness of time. But there are always tradeoffs...

marcan_42 5y ago

This isn't so much undefined behavior as unexpected statefulness, but yeah, it shouldn't have been designed like this. I've had a couple of run-ins with the Golang runtime myself, which were actual bugs. The first one was an issue with C interop triggered by my first production Go app; it had been reported but never successfully debugged, so I did so. It took a couple evenings to get it figured out, and a then a month to get the fix merged upstream. To be honest, that put me off of using Go for anything serious; I just never got the feeling that it's a solidly designed language and runtime. The second one I ran into as a user, and I think has ended up on HN a few times: https://marcan.st/2017/12/debugging-an-evil-go-runtime-bug/ https://marcan.st/2017/12/debugging-an-evil-go-runtime-bug/

barsonme 5y ago
Frankly, you did some amazing debugging. > and a then a month to get the fix merged upstream. To be honest, that put me off of using Go for anything serious; I just never got the feeling that it's a solidly designed language and runtime. This seems a little picky, though. Ian mailed the CL one day after you found the root cause. The CL was merged a couple days later. The fix was added to the very next patch release. That's a good turnaround time for a bug, especially a vDSO corner case. Go has its fair share of (sometimes quite annoying) oddities. In hindsight, some of the design and implementation choices were definitely incorrect. But they were still well thought out and I can't say I'd have made a better argument or decision at that point in time, either.
- marcan_42 5y ago
  That was the second bug. The first bug was this one (CL sent on 2014-08-19 and merged on 2014-09-24): https://github.com/golang/go/issues/7978 https://github.com/golang/go/issues/7978 https://codereview.appspot.com/131910043 https://codereview.appspot.com/131910043 I particularly like this comment: "Ping. Could somebody review this? Internal Google users suspect this is the cause of some of their problems." I guess once fixing Google production, always fixing Google production (I ran into this at my next job after leaving Google). But after that first bug, and having to dive deep into the golang runtime... it just didn't look well engineered to me, at all. Things like having architecture-specific details scattered open-coded all through stuff like the stack tracer (IIRC when they added ARM support they had to add an `lr` argument to a ton of codepaths). That first app was doing some Cgo stuff, and after looking carefully through how Cgo is supposed to work and how it actually works and what guarantees there are... it was just a mess. The only way to guarantee memory wouldn't be yanked out from under C was, apparently, to use malloc() and copy, which means you cannot do zero-copy calls into C code. There was no formal specification for what memory pinning is guaranteed or not. This is a general theme in Go. It feels like an ad-hoc language with a bunch of interesting design decisions, but largely superficial; in the end the deeper you dive into it, the more the answer to how things work is "shrug" (and "may change in the future"). It's clearly written by C people with a C mindset, trying to make a "better C" without letting go of the bad habits that C brings with it, and this is more evident the deeper you look. And I say this as primarily a C and Python coder. Then there's the whole "reinventing libc" insanity, even on macOS (where no such ABI stability was guaranteed, but they did it anyway, and that ended up with a macOS update breaking all Go apps). On Windows they can't get away with that, so they use Cgo instead, and then we're back at the Cgo mess/overhead. This design decision is also, ultimately, how the vDSO problem happened. I've also seen a tendency towards bloat in Go (see: the stories about Go binary size forever increasing). I find it particularly crazy that these days they need to have metadata that describes stack layout at every possible instruction in the program, to make their GC work. I don't mean all of this in a "Go is a bad language that should go away" sense; it has things it does well, and if I ever have to put together a high-performance concurrent network server with no external dependencies I might choose Go. No language is perfect. I just find that, after ending up deep in the bowels of Go, I'm not really inclined to default to it for anything that isn't very clearly its forte. I'm really looking forward to learning Rust, which seems much more serious in this regard, but I had a very different problem there; after writing a relatively simple Rust app once, I tried to engineer a more complex application in it, and ended up unable to wrap my head around how to make an abstract interface work with proper ownership rules. I should go back to doing something in Rust some day, something a bit less ambitious...

tapirl 5y ago

> Reads from a closed channel block, sends on a closed channel panic "never block"?

wahern 5y ago
Maybe they confused closed and nil channels. Reads from a nil channel block, as do sends.

zimpenfish 5y ago

Oh, this might explain the rare[1] local-only[2] "m=+0.00blah" test fails on deep `time.Time` comparisons we sometimes get talking to Firebase. (We don't actually use `time.Local` anywhere but I'm guessing one of the support libraries does.) [1] Once a month, maybe [2] But never on the CI - only when running the tests on developer machines

xyzzy_plugh 5y ago

This does not appear to be undefined at all. The issue is using reflect.DeepEqual to compare structs with unexported fields. The fields are an implementation detail, and comparing time.Time values is generally incorrect. Here are the actual docs from https://pkg.go.dev/time#Time.Equal https://pkg.go.dev/time#Time.Equal > Equal reports whether t and u represent the same time instant. Two times can be equal even if they are in different locations. For example, 6:00 +0200 and 4:00 UTC are Equal. See the documentation on the Time type for the pitfalls of using == with Time values; most code should use Equal instead. and > Time instants can be compared using the Before, After, and Equal methods. The Sub method subtracts two instants, producing a Duration. The Add method adds a Time and a Duration, producing a Time. and > Note that the Go == operator compares not just the time instant but also the Location and the monotonic clock reading. Therefore, Time values should not be used as map or database keys without first guaranteeing that the identical Location has been set for all values, which can be achieved through use of the UTC or Local method, and that the monotonic clock reading has been stripped by setting t = t.Round(0). In general, prefer t.Equal(u) to t == u, since t.Equal uses the most accurate comparison available and correctly handles the case when only one of its arguments has a monotonic clock reading. If I were reviewing this code, the bug squarely lands on the shoulders of the author who chose to use reflect.DeepEqual (or similar function available in many test helper libraries) here. Frankly, I find code that uses such blind comparisons to be lazy and prone to bugs exactly like this one. For readers unfamiliar with Go, this pattern is not unique to the time library. In general you avoid these sort of comparisons, at least without reading the docs. It's one thing if it's your own type or a fully-exported type or something obvious but this is just so clearly wrong. Furthermore I find this entire blog in pretty bad taste. Read the docs, own up to your own mistake, don't make blind assumptions about data types, don't lean on reflection to be lazy. Trying to throw shade at the Go authors crosses the line for me here, and name-calling Rob Pike is ultra immature. Trying to be cute, I guess? But ultimately I now think poorly of this business and its engineers. Bad form, folks.

cinntaile 5y ago
> Trying to throw shade at the Go authors crosses the line for me here, and name-calling Rob Pike is ultra immature. The whole blogpost is written in a tongue in cheek fashion, I don't think it's meant as name-calling.

hajhatten 5y ago

Why not just explicitly use UTC everywhere?

solidsnack9000 5y ago
Yeah, reading this article, it seemed like using a wildcard time zone was the real bug.

sgarman 5y ago

I feel bad for that new guy. I hope that was not the actual conversation.

papaf 5y ago
One thing I would recommend when someone blames a bug on you -- without hesitation, always say "yes, I will take a look at it". This way if you have to push back and say it wasn't you, you have much more impact because you have investigated the problem. Edit: but I agree, that is no way to treat a colleague.
exciteabletom 5y ago
>I demonstrated outstanding leadership and blamed him without any further evidence. At least the Author seems to know it wasn't OK.
sqldba 5y ago
Thanks! I felt the same way. Glad I’m not the only one. I’ve had people attack me like that before and it’s not great. It’s always someone in a position of power, too.
- shock-value 5y ago
  It’s embarrassing and shameful behavior by the author, and even more embarrassing that he was willing to share it publicly. If it’s the culture of that startup that this kind of stuff is just typical day-to-day employee interaction, I don’t know who would want to work there, especially given how hot the job market is now.

barsonme 5y ago

The problem here isn't with time.Time, it's with reflect.DeepEqual. Using it in your code to compare arbitrary data is almost always incorrect. And IMO, adding it to the stdlib was a mistake. time.Time values should only be compared for equality using the Equal method (or IsZero), but reflect.DeepEqual does not know that. This can happen with any type, not just time.Time. For example, I have a decimal library. It can represent 200 as either (mantissa=2, exponent=2) or (mantissa=200, exponent=0). Comparing those two numbers for equality will return true, but comparing them with reflect.DeepEqual will return false. So, it's not a "nasty bit of undefined timezone behavior." It's somebody misusing an API.

tialaramex 5y ago
This feels to me like maybe a good example for operator overloading. Often the "good" case of operator overloading is given as some fancy numeric type, like Complex or Matrix or something, for which some arithmetic makes sense but the fundamental integer and floating point operators don't cut it. But here is a case where programmer defined overloading of equality shows a benefit, if each type (including time.Time) takes responsibility for knowing how to compare that type then nobody else needs to know this detail and the natural comparison operators work, rather than needing to awkwardly define == and Equal separately and then forever live with people choosing the wrong one by mistake. After all, in the absence of such an overload (and I agree having it would interfere with Go's goal of simplicity) this would likely happen even without reflect.DeepEqual because people want to do this, and "You can't do this safely" is unlikely to have better results in Go than other languages.
- formerly_proven 5y ago
  This excerpt from the Go docs is an excellent case for (some) operating overloading: > Note that the Go == operator compares not just the time instant but also the Location and the monotonic clock reading. Therefore, Time values should not be used as map or database keys without first guaranteeing that the identical Location has been set for all values, which can be achieved through use of the UTC or Local method, and that the monotonic clock reading has been stripped by setting t = t.Round(0). In general, prefer t.Equal(u) to t == u, since t.Equal uses the most accurate comparison available and correctly handles the case when only one of its arguments has a monotonic clock reading.

jbaviat 5y ago

Je suis en train de me faire un devis pour

wereHamster 5y ago

Why is implicit global state still a thing in modern languages?

dblooman 5y ago

That message exchange screenshot is unfortunately seen too often in companies.

j16sdiz 5y ago

If you ask me, the problem lies in SQL datetime (without timezone) data type, not golang.