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

Inconsistent end_of_day result #4989

Closed
sebastianguarin opened this Issue Jan 17, 2018 · 10 comments

Comments

Projects
None yet
7 participants
@sebastianguarin

sebastianguarin commented Jan 17, 2018

Environment

  • jruby-9.1.15.0
  • rails 4.2.9
  • Darwin 14.5.0 Darwin Kernel Version 14.5.0: Sun Jun 4 21:40:08 PDT 2017; root:xnu-2782.70.3~1/RELEASE_X86_64 x86_64

Expected Behavior

  • The following occurs in MRI ruby:

Time.now.end_of_day => 2018-01-18 23:59:59 +1100

Actual Behavior

  • In Jruby-9.1.15.0:

jruby-9.1.15.0 > Time.now.end_of_day => 2018-01-19 00:00:00 +1100

@WToa

This comment has been minimized.

WToa commented Jan 18, 2018

Same issue here as well, reverting to 9.1.13 reports the correct date

@yaauie

This comment has been minimized.

yaauie commented Jan 18, 2018

Time#end_of_day is provided by ActiveSupport, and is somewhat orthogonal to this issue; below, I've replicated the root issue without ActiveSupport:

Time#end_of_day in ActiveSupport is implemented as

def end_of_day
    change(
      :hour => 23,
      :min => 59,
      :sec => 59,
      :usec => Rational(999999999, 1000)
    )
  end

-- active_support/core_ext/time/calculations.rb@v4.2.9

Where Time#change effectively passes the :usec option through as the usec parameter to either Time::utc or Time::local; it's this Rational that appears to be the source of our bug.

When either a Time is created via Time::utc or Time::local with a Rational value for its usec parameter that is sufficiently high, a rounding error shows up in jruby-9.1.15.0 that is not present in MRI 2.5.0 or jruby-9.1.12.0

╭─{ yaauie@castrovel:~ }
╰─○ RBENV_VERSION=jruby-9.1.15.0 ruby -rtime -ve 't = Time.local(2018,1,17,23,59,59, Rational(999999999,1000)); puts t; puts t.iso8601(6)'
jruby 9.1.15.0 (2.3.3) 2017-12-07 929fde8 Java HotSpot(TM) 64-Bit Server VM 9.0.4+11 on 9.0.4+11 +jit [darwin-x86_64]
2018-01-18 00:00:00 +0000
2018-01-18T00:00:00.000999+00:00
[success]

jruby-9.1.12.0 behaves as expected:

╭─{ yaauie@castrovel:~ }
╰─○ RBENV_VERSION=jruby-9.1.12.0 ruby -rtime -ve 't = Time.local(2018,1,17,23,59,59, Rational(999999999,1000)); puts t; puts t.iso8601(6)'
jruby 9.1.12.0 (2.3.3) 2017-06-15 33c6439 Java HotSpot(TM) 64-Bit Server VM 9.0.4+11 on 9.0.4+11 +jit [darwin-x86_64]
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.jruby.util.io.FilenoUtil to method sun.nio.ch.SelChImpl.getFD()
WARNING: Please consider reporting this to the maintainers of org.jruby.util.io.FilenoUtil
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
2018-01-17 23:59:59 +0000
2018-01-17T23:59:59.999999+00:00
[success]

MRI 2.5.0 behaves as expected:

╭─{ yaauie@castrovel:~ }
╰─○ RBENV_VERSION=2.5.0 ruby -rtime -ve 't = Time.local(2018,1,17,23,59,59, Rational(999999999,1000)); puts t; puts t.iso8601(6)'
ruby 2.5.0p0 (2017-12-25 revision 61468) [x86_64-darwin16]
2018-01-17 23:59:59 +0000
2018-01-17T23:59:59.999999+00:00
[success]

While I don't know the significance of this specific number, after briefly diving in more, it appears that Rational values somewhere north of Rational(999999877,1000):

╭─{ yaauie@castrovel:~ }
╰─○ RBENV_VERSION=jruby-9.1.15.0 ruby -rtime -ve 't = Time.local(2018,1,17,23,59,59, Rational(999999877,1000)); puts t; puts t.iso8601(6)'
jruby 9.1.15.0 (2.3.3) 2017-12-07 929fde8 Java HotSpot(TM) 64-Bit Server VM 9.0.4+11 on 9.0.4+11 +jit [darwin-x86_64]
2018-01-17 23:59:59 +0000
2018-01-17T23:59:59.999999+00:00
[success]

╭─{ yaauie@castrovel:~ }
╰─○ RBENV_VERSION=jruby-9.1.15.0 ruby -rtime -ve 't = Time.local(2018,1,17,23,59,59, Rational(999999878,1000)); puts t; puts t.iso8601(6)'
jruby 9.1.15.0 (2.3.3) 2017-12-07 929fde8 Java HotSpot(TM) 64-Bit Server VM 9.0.4+11 on 9.0.4+11 +jit [darwin-x86_64]
2018-01-18 00:00:00 +0000
2018-01-18T00:00:00.000999+00:00
[success]
@headius

This comment has been minimized.

Member

headius commented Feb 12, 2018

Still an issue as of today on jruby-9.1 HEAD.

@headius

This comment has been minimized.

Member

headius commented Feb 14, 2018

Ok, this is unlikely to be trivial, and it's not going to get fixed for 9.1.16.0.

The general problem is lack of good support for Rational time components. In this case the very small Rational usec is getting rounded up to 1s, which pushes the date overnight. I have not dug into the code, but I'm almost certain we're turning the Rational into a 64-bit floating point value, so the resolution at which this happens would be along that boundary.

The reason this won't be in 9.1.16.0 is because there's a lot of work involved in keeping usec (or really nsec) as a Rational if it came in as a Rational. On this timeframe, that's too big a risk.

I suspect the eventual fix for this will be to turn the rational into a BigDecimal and store that internally, so we still have proper numeric operations but do not have to preserve all the Ruby semantics and overhead that go with a Rational object.

@headius

This comment has been minimized.

Member

headius commented Feb 14, 2018

Perhaps someone can try to bisect between 9.1.13 and 9.1.14 to see which commit(s) introduced this regression? It's probably going to be a fix for some other issue, but at least we'd have a narrower focus.

@mattbooks-square

This comment has been minimized.

mattbooks-square commented Mar 20, 2018

Looks like this was introduced in 4be8d66b9e91e03e7f010b64a19c5290e7f651ec

@headius

This comment has been minimized.

Member

headius commented Mar 26, 2018

@mattbooks-square Of course it was 😢

@headius

This comment has been minimized.

Member

headius commented Mar 26, 2018

@kares I don't suppose any work you've done might help this? I think you've mainly been messing with Date...

@kares

This comment has been minimized.

Member

kares commented Mar 27, 2018

nope, but I know why we regressed - that was an attempt to deal with a rounding issue, probably.
(which I reported from AR-JDBC) ... will take a look

kares added a commit to kares/jruby that referenced this issue Mar 27, 2018

@kares kares self-assigned this Mar 27, 2018

@kares

This comment has been minimized.

Member

kares commented Mar 27, 2018

resolved in jruby-9.1 with 682626e ...

@kares kares closed this Mar 27, 2018

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