Issue 5985059: code review 5985059: time: panic if UnixNano is out of range (Closed)
Descriptiontime: panic if UnixNano is out of range
Patch Set 1 #Patch Set 2 : code review 5985059: time: panic if UnixNano is out of range #Patch Set 3 : diff -r 734071724054 https://go.googlecode.com/hg/ #Patch Set 4 : diff -r 734071724054 https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 5 : diff -r 734071724054 https://go.googlecode.com/hg/ #
Total comments: 3
Patch Set 6 : diff -r 734071724054 https://go.googlecode.com/hg/ #Patch Set 7 : diff -r 734071724054 https://go.googlecode.com/hg/ #Patch Set 8 : diff -r 734071724054 https://go.googlecode.com/hg/ #Patch Set 9 : diff -r 734071724054 https://go.googlecode.com/hg/ #Patch Set 10 : diff -r 734071724054 https://go.googlecode.com/hg/ #Patch Set 11 : diff -r 734071724054 https://go.googlecode.com/hg/ #Patch Set 12 : diff -r 734071724054 https://go.googlecode.com/hg/ #Patch Set 13 : diff -r 734071724054 https://go.googlecode.com/hg/ #Patch Set 14 : diff -r 734071724054 https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 15 : diff -r 734071724054 https://go.googlecode.com/hg/ #
Total comments: 1
Patch Set 16 : diff -r 344d5c33331a https://go.googlecode.com/hg/ #MessagesTotal messages: 31
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
http://codereview.appspot.com/5985059/diff/4/src/pkg/time/time.go File src/pkg/time/time.go (right): http://codereview.appspot.com/5985059/diff/4/src/pkg/time/time.go#newcode775 src/pkg/time/time.go:775: if t.sec > maxUnixNano || t.sec < minUnixNano { isn't it t.sec + internalToUnix that you want to test? What if the result is 1<<63 + 1 nanoseconds?
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
http://codereview.appspot.com/5985059/diff/4/src/pkg/time/time.go File src/pkg/time/time.go (right): http://codereview.appspot.com/5985059/diff/4/src/pkg/time/time.go#newcode775 src/pkg/time/time.go:775: if t.sec > maxUnixNano || t.sec < minUnixNano { On 2012/04/07 18:30:23, remyoudompheng wrote: > isn't it t.sec + internalToUnix that you want to test? > What if the result is 1<<63 + 1 nanoseconds? Sorry, I had already realized it was actually wrong, but hadn't updated it. I had updated the constants instead, although what you suggest would work too.
Sign in to reply to this message.
Ouch. This seems a bit harsh. Why not just let it overflow? Even besides whether it should panic, this seems like an API change.
Sign in to reply to this message.
On Sat, Apr 7, 2012 at 20:20, David Symonds <dsymonds@golang.org> wrote: > Ouch. This seems a bit harsh. Why not just let it overflow? Can you find some reason why such a result would be useful rather than being a silly user error that silently results in a garbage timestamp and thus ought to be fixed? > Even besides whether it should panic, this seems like an API change. It's already a bug to use that value today. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
On Sun, Apr 8, 2012 at 9:34 AM, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: >> Even besides whether it should panic, this seems like an API change. > > It's already a bug to use that value today. Yes, but people still depend on bugs behaving in a certain way. Right now, they get a weird number that may or may not be a problem for their particular situation; after this change, their program panics.
Sign in to reply to this message.
On Sat, Apr 7, 2012 at 20:39, David Symonds <dsymonds@golang.org> wrote: > On Sun, Apr 8, 2012 at 9:34 AM, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: > >>> Even besides whether it should panic, this seems like an API change. >> >> It's already a bug to use that value today. > > Yes, but people still depend on bugs behaving in a certain way. Right > now, they get a weird number that may or may not be a problem for > their particular situation; after this change, their program panics. I'm part of that "people" set you talk about, I had such a bug in my code, and I'm sad to find it with a bug report rather than with Go telling me about it. Trivia: guess what this date means: 1754-08-30 22:43:41.128654848 +0000 UTC Silently allowing that kind of garbage to go through is worth a panic, IMO. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
On 2012/04/08 00:05:03, gustavo_niemeyer.net wrote: > Silently allowing that kind of garbage to go through is worth a panic, IMO. time.Unix() is documented to allow out-of-range nanoseconds: It is valid to pass nsec outside the range [0, 999999999]. Considering this, I think the right thing to do is document the possible overflow in UnixNano() instead of panicking.
Sign in to reply to this message.
On Sun, Apr 8, 2012 at 17:42, <dchest@gmail.com> wrote: > time.Unix() is documented to allow out-of-range nanoseconds: > > It is valid to pass nsec outside the range [0, 999999999]. > > Considering this, I think the right thing to do is document the possible > overflow in UnixNano() instead of panicking. These two facts are unrelated. time.Unix handles *correctly* out of range nanoseconds. To be honest, though, if I'm the odd one that thinks this is a trap, then it should probably remain unchanged indeed. I for one am not falling on it again. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
On 2012/04/08 21:55:48, gustavo_niemeyer.net wrote: > On Sun, Apr 8, 2012 at 17:42, <mailto:dchest@gmail.com> wrote: > > time.Unix() is documented to allow out-of-range nanoseconds: > > > > It is valid to pass nsec outside the range [0, 999999999]. > > > > Considering this, I think the right thing to do is document the possible > > overflow in UnixNano() instead of panicking. > > These two facts are unrelated. time.Unix handles *correctly* out of > range nanoseconds. You're right, I'm withdrawing this argument. > To be honest, though, if I'm the odd one that thinks this is a trap, > then it should probably remain unchanged indeed. I for one am not > falling on it again. If so, the fact that UnixNano can't handle some dates should be documented, though. Right now it's a trap in which I, coincidentally, fell today. For my program, the wrong date would be okay, but panicking in the next release of Go 1 would be bad. However, if someone is writing a time machine in Go, the fact the it will send her into year 1754 instead of 1 would be catastrophic, so panicking here is better.
Sign in to reply to this message.
http://codereview.appspot.com/5985059/diff/5/src/pkg/time/time.go File src/pkg/time/time.go (right): http://codereview.appspot.com/5985059/diff/5/src/pkg/time/time.go#newcode775 src/pkg/time/time.go:775: if t.sec > maxUnixNano || t.sec < minUnixNano { i don't believe this is correct. the > should be >=. a test would verify. also range tests like this read better in this style, with the variable outside the "range" represented by the constants. t.sec < minUnixNano || maxUnixNano <= t.sec this is not to pass judgement on having this panic. i'm thinking about that.
Sign in to reply to this message.
PTAL http://codereview.appspot.com/5985059/diff/5/src/pkg/time/time.go File src/pkg/time/time.go (right): http://codereview.appspot.com/5985059/diff/5/src/pkg/time/time.go#newcode775 src/pkg/time/time.go:775: if t.sec > maxUnixNano || t.sec < minUnixNano { On 2012/04/09 06:00:28, r wrote: > i don't believe this is correct. the > should be >=. a test would verify. > > also range tests like this read better in this style, with the variable outside > the "range" represented by the constants. > > t.sec < minUnixNano || maxUnixNano <= t.sec The range was off by one indeed, but to the other side. There was one more value that should not panic, while changing the equality here would mean panicking to yet another value (would be off by 2). I've simplified the constants to make them more clear, added tests verifying both sides of the boundary, and also improved the comment to mention the most likely bug coming out of this (the zero time). > this is not to pass judgement on having this panic. i'm thinking about that. Sounds good.
Sign in to reply to this message.
http://codereview.appspot.com/5985059/diff/5/src/pkg/time/time.go File src/pkg/time/time.go (right): http://codereview.appspot.com/5985059/diff/5/src/pkg/time/time.go#newcode775 src/pkg/time/time.go:775: if t.sec > maxUnixNano || t.sec < minUnixNano { On 2012/04/09 06:00:28, r wrote: > also range tests like this read better in this style, with the variable outside > the "range" represented by the constants. Sorry, reordered the comparison now as well.
Sign in to reply to this message.
I am not sure that I'm comfortable with something this low-level panicking. I think I would rather just clamp the result or even just define that it is undefined outside the useful range. I am also not sure that the test is correct. It is checking whether the seconds can be represented as nanoseconds, but it does not check whether adding nsec then causes an overflow. Russ
Sign in to reply to this message.
On Mon, Apr 9, 2012 at 15:00, Russ Cox <rsc@golang.org> wrote: > I am not sure that I'm comfortable with something this low-level panicking. > I think I would rather just clamp the result or even just define that it is > undefined outside the useful range. > > I am also not sure that the test is correct. It is checking whether the > seconds can be represented as nanoseconds, but it does not check > whether adding nsec then causes an overflow. You're right.. the logic I implemented would only work if the defined range for nsec was binary rather than decimal. Clearly I suck. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
On Mon, Apr 9, 2012 at 15:15, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: > You're right.. the logic I implemented would only work if the defined > range for nsec was binary rather than decimal. Clearly I suck. Rather than going through the trouble of increasing the complexity of the overflow check by putting the nsec value in business, I went with a trivial approach of sign comparison. I think that's feasible given the guarantees of the spec, but I wouldn't bet on it at this point anymore. Even if that's going to the trash, can you please have a look so I have a chance at getting back at least a low percentage of my lost self esteem? Thanks, -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
Le 9 avril 2012 21:12, Gustavo Niemeyer <gustavo@niemeyer.net> a écrit : > On Mon, Apr 9, 2012 at 15:15, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: >> You're right.. the logic I implemented would only work if the defined >> range for nsec was binary rather than decimal. Clearly I suck. > > Rather than going through the trouble of increasing the complexity of > the overflow check by putting the nsec value in business, I went with > a trivial approach of sign comparison. I think that's feasible given > the guarantees of the spec, but I wouldn't bet on it at this point > anymore. t, _ := time.Parse("20060102", "11060928") fmt.Println(t.Unix()) fmt.Println(t.UnixNano()) It will not detect this overflow, neither in year 2555 or so.
Sign in to reply to this message.
On Mon, Apr 9, 2012 at 16:25, Rémy Oudompheng <remyoudompheng@gmail.com> wrote: > t, _ := time.Parse("20060102", "11060928") > fmt.Println(t.Unix()) > fmt.Println(t.UnixNano()) > > It will not detect this overflow, neither in year 2555 or so. Gosh.. I'll be hiding under a rock studying overflows if someone needs me. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, remyoudompheng@gmail.com, dsymonds@golang.org, gustavo@niemeyer.net, dchest@gmail.com, r@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On Mon, Apr 9, 2012 at 18:12, <n13m3y3r@gmail.com> wrote: > Please take another look. > http://codereview.appspot.com/5985059/ I hope this makes sense now.. phew. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
http://codereview.appspot.com/5985059/diff/5010/src/pkg/time/time.go File src/pkg/time/time.go (right): http://codereview.appspot.com/5985059/diff/5010/src/pkg/time/time.go#newcode785 src/pkg/time/time.go:785: } I suggest either clamping the value or maybe providing a predefined constant time for the error case. Document it too. If you want a panic, it might be better to create a second function that panics.
Sign in to reply to this message.
http://codereview.appspot.com/5985059/diff/5010/src/pkg/time/time.go File src/pkg/time/time.go (right): http://codereview.appspot.com/5985059/diff/5010/src/pkg/time/time.go#newcode785 src/pkg/time/time.go:785: } On 2012/04/09 21:19:06, r wrote: > I suggest either clamping the value or maybe providing a predefined > constant time for the error case. Document it too. The most problematic case that will show up every once in a while in normal applications is the zero value being used as an "unset" flag. Either of those options will continue mishandling that. That said, I can move on with either of those to at least improve the current behavior, if you wish. Please pick one and I'll move on with it. > If you want a panic, it might be better to create a second function that > panics. I don't actually want a panicking function. My intention was to prevent people from doing silly mistakes with the API offered. If we were not past the Go 1 point, I'd suggest either adding an additional error result, or simply deprecating the function completely to avoid having an API that works in part of the supported range only. It's too late, though.
Sign in to reply to this message.
I am unconvinced that is worth worrying about.
I would document that if the result does not fit in an
int64 then the returned value is undefined and call it a day.
If we must do something, then detecting the overflow
and clamping the result should be easy:
secs := t.sec + internalToUnix
nsec := secs * 1e9
if nsec/1e9 != secs || nsec+t.nsec < nsec {
if t.sec > 0 {
return 1<<63 - 1
}
return -1<<63
}
return nsec+t.nsec
This works because 0 <= t.nsec < 1e9.
Note that even this doesn't handle the case where you've
constructed a date so far in the future where t.sec + internalToUnix
overflows. I really don't think this is worth worrying about in such
detail.
Russ
Sign in to reply to this message.
On Mon, Apr 9, 2012 at 18:44, Russ Cox <rsc@golang.org> wrote: > I am unconvinced that is worth worrying about. > I would document that if the result does not fit in an > int64 then the returned value is undefined and call it a day. Given the range we live under at the moment, I agree it is almost irrelevant. The main practical problem for non-historians comes out of the zero value, though, which is used frequently and falls out of the range. Serializing it as a UnixNano doesn't sound like a stretch. > If we must do something, then detecting the overflow > and clamping the result should be easy: > > secs := t.sec + internalToUnix > nsec := secs * 1e9 > if nsec/1e9 != secs || nsec+t.nsec < nsec { n := int64(math.MaxInt64/2 + 1) a := n*1e9 b := a/1e9 println(a == b) Shall we have a beer tonight? -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
On Mon, Apr 9, 2012 at 18:15, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: > On Mon, Apr 9, 2012 at 18:44, Russ Cox <rsc@golang.org> wrote: >> I am unconvinced that is worth worrying about. >> I would document that if the result does not fit in an >> int64 then the returned value is undefined and call it a day. > > Given the range we live under at the moment, I agree it is almost > irrelevant. The main practical problem for non-historians comes out of > the zero value, though, which is used frequently and falls out of the > range. Serializing it as a UnixNano doesn't sound like a stretch. > >> If we must do something, then detecting the overflow >> and clamping the result should be easy: >> >> secs := t.sec + internalToUnix >> nsec := secs * 1e9 >> if nsec/1e9 != secs || nsec+t.nsec < nsec { > > n := int64(math.MaxInt64/2 + 1) > a := n*1e9 > b := a/1e9 > println(a == b) I don't understand. My code, translated to use your variables, is testing a/1e9 == n, not a == a/1e9. Russ
Sign in to reply to this message.
On Mon, Apr 9, 2012 at 19:21, Russ Cox <rsc@golang.org> wrote: > I don't understand. My code, translated to use your variables, > is testing a/1e9 == n, not a == a/1e9. Sorry, I had missed the underlying reasoning, unsurprisingly. It's a very nice approach, thanks for shedding some light. I've updated the CL so it only changes the documentation. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything.
Sign in to reply to this message.
LGTM http://codereview.appspot.com/5985059/diff/2014/src/pkg/time/time.go File src/pkg/time/time.go (right): http://codereview.appspot.com/5985059/diff/2014/src/pkg/time/time.go#newcode766 src/pkg/time/time.go:766: // since January 1, 1970 UTC. The result is undefined if representing t The result is undefined if the Unix time in nanoseconds cannot be represented by an int64. Note that this means the result of calling UnixNano on the zero Time is undefined.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=4ed98a6b6fe5 *** time: panic if UnixNano is out of range R=golang-dev, remyoudompheng, dsymonds, gustavo, dchest, r, rsc CC=golang-dev http://codereview.appspot.com/5985059
Sign in to reply to this message.
On 2012/04/13 01:16:54, niemeyer wrote: > *** Submitted as http://code.google.com/p/go/source/detail?r=4ed98a6b6fe5 *** > > time: panic if UnixNano is out of range For the record, this is wrong. It was a documentation change only.
Sign in to reply to this message.
> For the record, this is wrong. It was a documentation change only. phew doc change, a+ adding panic ... i would prefer we did less of that on core pkgs (passing stupid data in most cases deserves stupid results, but ideally don't panic)
Sign in to reply to this message.
|
