Time.at broken in 1.7.3 when passed (Time, 0) #565

Closed
matpowel opened this Issue Mar 9, 2013 · 7 comments

Projects

None yet

4 participants

@matpowel
matpowel commented Mar 9, 2013

Hi guys,

Great job on JRuby, we're making the switch and it's super fast in 1.8 mode anyway.

Here's a bug we've hit in our test suite though:

irb
irb(main):001:0> Time.at(0)
# => Wed Dec 31 17:00:00 -0700 1969
irb(main):002:0> Time.at(0, 0)
# => Wed Dec 31 17:00:00 -0700 1969
irb(main):003:0> Time.at(Time.at(0), 0)
#TypeError: can't convert Time into Integer
#   from org/jruby/RubyTime.java:983:in `at'
#   from (irb):3:in `evaluate'
#   from org/jruby/RubyKernel.java:1061:in `eval'
irb(main):004:0> Time.at(Time.at(0))
#=> Wed Dec 31 17:00:00 -0700 1969

If you're curious, we came across it because of the bug I referenced at travisjeffery/timecop#61 when trying to use the Timecop gem in JRuby when ruby_units is also loaded. This is one of those blame game things but here's my synopsis:

  1. The is obviously a definite bug in JRuby
  2. When ruby_units is included, it passes 0 as the second parameter instead of nothing which makes it technically not identical to the original Time class thus exposing the bug, I don't think ruby_units should be monkey patching the Time class and if it does it should make sure the behavior is absolutely identical. This isn't the first time we've had that problem with ruby_units olbrich/ruby-units#48, but I still love the gem (thanks @olbrich!)
  3. Timecop is an innocent bystander in the whole thing, it's just trying to use Time.at..
  4. I'm quite confused after looking at the 1.7.3 tagged code for RubyTime (https://github.com/jruby/jruby/blob/1.7.3/src/org/jruby/RubyTime.java) because I can't see how passing arg2 changes the behavior of line 983, which makes me think your 1.7.3 tag is wrong in Github and the code that's running is different?

For anyone reading who happens to hit the same problem (at least @glebm mentioned it in another issue) I'm guessing we'll have to fork or monkeypatch ruby-units to not force that second parameter being passed through until JRuby is fixed. I'll update with my solution when I get a chance.

@matpowel
matpowel commented Mar 9, 2013

Ok so for anyone who's interested, here's the solution I came up with. Rather than patching ruby-units I decided to patch the whole of Time.at because it could very well show up within other gems under JRuby 1.7.3 (and possibly other versions I haven't tested)..

https://gist.github.com/matpowel/5123358

@BanzaiMan
Member

https://github.com/ruby/ruby/blob/v1_9_3_392/time.c#L2520 shows that we should have a special case for Time object as the first argument. This is a JRuby bug, though the documentation (https://github.com/ruby/ruby/blob/v1_9_3_392/time.c#L2487-L2506) is a bit off.

@BanzaiMan BanzaiMan was assigned Mar 19, 2013
@sluukkonen
Contributor

Looks like MRI will accept a Time object as the second argument as well. Not quite sure what the exact semantics are yet, but it seem to do the equivalent of calling #to_f on the second argument (which doesn't make any sense to me, as #to_f will return a value in seconds, while the second argument should be a value in microseconds).

sluukkonen@ubuntu:~$ irb
irb(main):001:0> Time.at(0, Time.at(0, 123456.789))
=> 1970-01-01 02:00:00 +0200
irb(main):002:0> Time.at(0, Time.at(0, 123456.789)).nsec
=> 123
irb(main):003:0> Time.at(0, Time.at(0, 123456.789).to_f).nsec
=> 123
irb(main):004:0> RUBY_VERSION
=> "2.0.0"
@BanzaiMan
Member

It seems to me that if there are two arguments, MRI is just adding them (the first argument is converted to the seconds since the epoch, the second argument is converted as nanoseconds since the epoch).

1.9.3p392 :001 > Time.at(1000000)
 => 1970-01-12 08:46:40 -0500 
1.9.3p392 :002 > Time.at(2000000)
 => 1970-01-23 22:33:20 -0500 
1.9.3p392 :003 > Time.at(1000000,1000000_000000)
 => 1970-01-23 22:33:20 -0500 
1.9.3p392 :004 > Time.at(1000000,Time.at(1000000_000000))
 => 1970-01-23 22:33:20 -0500
1.9.3p392 :005 > Time.at(1000000,Time.at(1000000_000000)) == Time.at(2000000)
 => true 
1.9.3p392 :006 > Time.at(1000000,Time.at(1000000_000001)) == Time.at(2000000)
 => false 
@matpowel

@BanzaiMan yep that's the behavior I enforced/emulated in my monkey patch above and it seems compatible with MRI

@BanzaiMan BanzaiMan added a commit that closed this issue Mar 27, 2013
@BanzaiMan BanzaiMan Fix #565.
The 2-arg form of `Time#at` can take a `Time` as either argument, and basically performs a time addition.

It should be possible to clean up this somewhat convoluted logic. Patch welcome. :-)
b67c1a6
@BanzaiMan BanzaiMan closed this in b67c1a6 Mar 27, 2013
@BanzaiMan
Member

See also: http://bugs.ruby-lang.org/issues/8173 and rubyspec/rubyspec#190.

@likethesky

See also: #755 as I'm getting a similar error with 1.7.4 (that has this fix).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment