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

Date.today does not appear to take the local TZ into account #5418

Closed
raelik opened this Issue Nov 6, 2018 · 30 comments

Comments

Projects
None yet
5 participants
@raelik
Copy link

raelik commented Nov 6, 2018

Environment

jRuby 9.2.0.0+ (may also happen on prior versions)
Tested on OS X and Linux.

Expected Behavior

irb(main):001:0> Time.now
=> 2018-11-05 23:32:54 -0600
irb(main):002:0> Date.today
=> #<Date: 2018-11-05 ((2458428j,0s,0n),+0s,2299161j)>

Actual Behavior

irb(main):001:0> Time.now
=> 2018-11-05 23:32:54 -0600
irb(main):002:0> Date.today
=> #<Date: 2018-11-06 ((2458428j,0s,0n),+0s,2299161j)>

What appears to be happening here is that since the DST shift occurred on 11/04 at 2am, after 11pm each day thereafter (presumably until DST begins again in March), Date.today returns the next day. This does not happen if doing Time.now.to_date.

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Nov 6, 2018

@raelik I see the same actual behavior using C Ruby as well. If you can confirm can you open an issue on ruby-lang.org on this?

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 6, 2018

@enebo I believe the expected behavior here is CRuby, so perhaps this is some environment difference we're not handling right?

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Nov 6, 2018

@headius I am just saying I see the same actual using MRI and JRuby on my linux box.

@kares

This comment has been minimized.

Copy link
Member

kares commented Nov 7, 2018

well that's a bug - shall we wait a year to try reproducing :) ?
hopefully, @raelik will be able to provide a reproduction script which fails for him these days ...

jRuby 9.2.0.0+

and since there was a Date rewrite do you recall if this happened for 9.1 as well?

@raelik

This comment has been minimized.

Copy link
Author

raelik commented Nov 14, 2018

Here is my "reproduction script":

xardion@arkham:~$ java -version
java version "1.8.0_161"
Java(TM) SE Runtime Environment (build 1.8.0_161-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.161-b12, mixed mode)

xardion@arkham:~$ ruby -version
ruby 2.3.3p222 (2016-11-21) [x86_64-linux-gnu]

xardion@arkham:~$ java -jar jruby-complete-9.2.0.0.jar -S irb
irb(main):001:0> require 'date'
=> true
irb(main):002:0> Date.today
=> #<Date: 2018-11-14 ((2458437j,0s,0n),+0s,2299161j)>
irb(main):003:0> quit

xardion@arkham:~$ irb
irb(main):001:0> require 'date'
=> true
irb(main):002:0> Date.today
=> #<Date: 2018-11-13 ((2458436j,0s,0n),+0s,2299161j)>
irb(main):003:0> quit

xardion@arkham:~$ java -jar jruby-complete-9.1.17.0.jar -S irb
irb(main):001:0> require 'date'
=> true
irb(main):002:0> Date.today
=> #<Date: 2018-11-13 ((2458436j,0s,0n),+0s,2299161j)>
irb(main):003:0> quit

@raelik raelik changed the title Date.today does not appear to take DST into account Date.today does not appear to take the local TZ into account Nov 14, 2018

@raelik

This comment has been minimized.

Copy link
Author

raelik commented Nov 14, 2018

So I ran the above on my Ubuntu Linux 17.10 box, on 11/13 @ 8:20pm CST. This seems to be due to Date.today not taking my local TZ into account. I tested this right before 6pm, and right after, and that is when the behavior begins. I'm pretty sure the culprit is here: https://github.com/jruby/jruby/blob/9.2.0.0/core/src/main/java/org/jruby/ext/date/RubyDate.java#L719

This shouldn't be using CHRONO_ITALY_UTC, as the MRI Date.today should be taking local TZ into account. This might be necessary in other places, but there probably needs to be a CHRONO_ITALY that uses the local TZ, likely initialized with GJChronology.getInstance(), as the no argument version uses the default TZ.

@raelik

This comment has been minimized.

Copy link
Author

raelik commented Nov 14, 2018

Using UTC when generating the calendar offset is definitely the culprit:

xardion@arkham:~$ java -jar jruby-complete-9.2.0.0.jar -S irb
irb(main):001:0> require 'date'
=> true
irb(main):002:0> Time.now
=> 2018-11-13 20:53:35 -0600
irb(main):003:0> Date.today
=> #<Date: 2018-11-14 ((2458437j,0s,0n),+0s,2299161j)>
irb(main):004:0> dt = org.joda.time.DateTime.new(org.joda.time.chrono.GJChronology.getInstance()).withTimeAtStartOfDay()
=> #<Java::OrgJodaTime::DateTime:0x7e307087>
irb(main):005:0> org.jruby.ext.date.RubyDate.new(JRuby.runtime, dt)
=> #<Date: 2018-11-13 ((2458436j,0s,0n),+0s,2299161j)>
@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 18, 2018

Does this patch fix it?

diff --git a/core/src/main/java/org/jruby/ext/date/RubyDate.java b/core/src/main/java/org/jruby/ext/date/RubyDate.java
index 8ed5347cf7..7777c5fb5c 100644
--- a/core/src/main/java/org/jruby/ext/date/RubyDate.java
+++ b/core/src/main/java/org/jruby/ext/date/RubyDate.java
@@ -78,7 +78,7 @@ public class RubyDate extends RubyObject {
 
     static final Logger LOG = LoggerFactory.getLogger(RubyDate.class);
 
-    static final GJChronology CHRONO_ITALY_UTC = GJChronology.getInstance(DateTimeZone.UTC);
+    static final GJChronology CHRONO_ITALY_UTC = GJChronology.getInstance(RubyTime.getLocalTimeZone());
 
     // The Julian Day Number of the Day of Calendar Reform for Italy
     // and the Catholic countries.

@headius headius added this to the JRuby 9.2.5.0 milestone Nov 18, 2018

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 18, 2018

Sorry, that doesn't appear to compile for me. I'll get a new patch shortly.

@kares

This comment has been minimized.

Copy link
Member

kares commented Nov 18, 2018

believe CHRONO_ITALY_UTC is really assumed in a few places to be for the UTC time-zone

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 18, 2018

@kares Yeah I'm trying to match up this logic with that in MRI but they don't have this extra constant, or at least nothing like it is used in Date.today.

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 19, 2018

I think this patch may do it?

diff --git a/core/src/main/java/org/jruby/ext/date/RubyDate.java b/core/src/main/java/org/jruby/ext/date/RubyDate.java
index 8ed5347cf7..68b08984f7 100644
--- a/core/src/main/java/org/jruby/ext/date/RubyDate.java
+++ b/core/src/main/java/org/jruby/ext/date/RubyDate.java
@@ -713,7 +713,7 @@ public class RubyDate extends RubyObject {
 
     @JRubyMethod(meta = true)
     public static RubyDate today(ThreadContext context, IRubyObject self) { // sg=ITALY
-        return new RubyDate(context.runtime, (RubyClass) self, new DateTime(CHRONO_ITALY_UTC).withTimeAtStartOfDay());
+        return new RubyDate(context.runtime, (RubyClass) self, new DateTime(getChronology(context, ITALY, 0)).withTimeAtStartOfDay());
     }
 
     @JRubyMethod(meta = true)

headius added a commit to headius/jruby that referenced this issue Nov 19, 2018

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 20, 2018

This still needs a test, but I'm not sure how to do that without setting the time of the host machine 😢

@headius headius reopened this Nov 21, 2018

@headius

This comment has been minimized.

Copy link
Member

headius commented Nov 21, 2018

Apparently my fix did not do it...

@raelik

This comment has been minimized.

Copy link
Author

raelik commented Nov 21, 2018

Sorry for not being more proactive here, was out of pocket for a bit there. So this fix just moved the issue. CHRONO_ITALY_UTC is the problem here, and getChronology(context, ITALY, 0) just returns CHRONO_ITALY_UTC if you call it with those args:

    static Chronology getChronology(ThreadContext context, final long sg, final int off) {
        final DateTimeZone zone;
        if (off == 0) {
            if (sg == ITALY) return CHRONO_ITALY_UTC;
            zone = DateTimeZone.UTC;
        }
        else {

headius added a commit to headius/jruby that referenced this issue Nov 21, 2018

@enebo enebo modified the milestones: JRuby 9.2.5.0, JRuby 9.2.6.0 Dec 6, 2018

@headius headius modified the milestones: JRuby 9.2.6.0, JRuby 9.2.7.0 Dec 18, 2018

@enebo enebo removed this from the JRuby 9.2.7.0 milestone Dec 19, 2018

@enebo enebo added this to the JRuby 9.2.7.0 milestone Jan 9, 2019

@kstephens

This comment has been minimized.

Copy link

kstephens commented Jan 13, 2019

This problem serious enough, and simple enough, that it should not wait until 9.2.7.0.
Please issue a 9.2.5.1 release.

It broke in 9.2.1.0, 9.1.17.0 does not have this problem.
This fails with multiple JDKs, on OS X and Linux.
The failure is unaffected by setting $TZ.

For example, after 6pm in CDT timezone (UTC-6:00), Time.now(.year, .month, .day) is one day behind Date.today (.year, .month. day). Before 6pm CDT, Date.today == Time.now.to_date

If anyone is interested, I have a test suite across many JRuby/JDK versions.

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Jan 14, 2019

@kstephens The current PR does not fully fix out date woes. Some tests in sequel do not like this change so we need some help in figuring out why that is. We can definitely move this up from 9.2.7.0 if we have a good fix but the current one MAYBE is not right (it is possible sequel has some code which works around something with out handling or something).

@kstephens

This comment has been minimized.

Copy link

kstephens commented Jan 15, 2019

@enebo Thanks for your reply.

Perhaps there's something subtle I don't understand. If Date.today == Time.now.to_date was true since 9.1.17, did MRI (as some version) change Date.today to be in UTC? I found references to a Date.current method which seemed to exist at one point (in activesupport?). Very old versions of JRuby (e.g. 1.6.5) Date.today was in local TZ. I haven't confirmed the history of MRI's Date.today behavior, yet.

Background: We are migrating the first of many apps from JRuby 1.6.5 to 9.2.5.0 (yea, I know it's a long-haul) -- this difference really tripped us up. We considered falling back to 9.1.17.0, but we already spent significant time migrating to some Ruby 2.5.0 semantics. As of yesterday, we decided to monkey-patch Date.today on 9.2 and not recertify our app on 9.1.

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Jan 15, 2019

@raelik do you have a better fix for this? You implied this moved the problem and we can see sequel is failing with just this fix.

@kstephens we will try and get a proper fix in soon. Depending on the outstanding refinements fixes (which is fairly critical since i18n which installs by default does not work) I would put this fix into 9.1.6.0. Which we had originally planned to do just before xmas and decided that is a scary time to release. 9.1.5.1 is a possibility but just not as desirable.

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Jan 15, 2019

Err I should have said above any gems like rails which depends on i18n will pull in the refinements version which is not working on any version of JRuby.

@raelik

This comment has been minimized.

Copy link
Author

raelik commented Jan 15, 2019

@enebo My suggestion was to not use CHRONO_ITALY_UTC in the today implementation, but to have another constant (sayyy CHRONO_ITALY) that is initialized with GJChronology.getInstance() (EXACTLY that, with no arguments). You could probably just call that directly if another constant isn't desirable, but I think a constant would be more efficient.

@kstephens

This comment has been minimized.

Copy link

kstephens commented Jan 15, 2019

@enebo I wasn't aware of the i18n Refinements issue. Now I understand why it would take priority over something that can be fixed a monkey-patch on our side.

enebo added a commit that referenced this issue Jan 16, 2019

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Jan 16, 2019

@raelik like ba2d8e6? I am loathe to set my system time to check this out and I am curious if we can find a reasonable way to test this...

@raelik

This comment has been minimized.

Copy link
Author

raelik commented Jan 16, 2019

@enebo Yeah, that'll do it. You don't have to reset your time to test it... but you do have to wait until the difference between your local time and UTC will push one of them over the boundary.

enebo added a commit that referenced this issue Jan 17, 2019

@enebo enebo modified the milestones: JRuby 9.2.7.0, JRuby 9.2.6.0 Jan 17, 2019

@enebo enebo closed this Jan 17, 2019

@kstephens

This comment has been minimized.

Copy link

kstephens commented Jan 18, 2019

@enebo I bumped a VM clock because I don't like working after 6pm US/CDT. :)
BTW: I tried using libfaketime but that doesn't work with any JDKs I tried.

@kares

This comment has been minimized.

Copy link
Member

kares commented Jan 27, 2019

as noted: #5560 (comment) there seems to be a regression with the (latest) fix.
hate reverting stuff but if there isn't a proper fix we shall consider doing so for 9.2.6 ... the reason being that Date.today.to_time.to_date round-trip seems to be pretty important in Rails (and likely others),
since the change that piece of code no longer works, at least for me in CET:

$ date
Sun 27 Jan 21:04:45 CET 2019
$ date -u
Sun 27 Jan 20:04:48 UTC 2019

... haven't investigated this one much further as I am in the middle of looking into a few others

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Jan 28, 2019

@kares yeah I think we need something working for this even if if includes reverting back to 9.1.x when it was working. The fixes do keep getting closer... :|

@kares kares reopened this Jan 29, 2019

@kares

This comment has been minimized.

Copy link
Member

kares commented Jan 29, 2019

re-opened since the issue seems to need more work.
for the record 9.1.17.0 did use UTC zone for Date.today (its not my local zone) :

jruby-9.1.17.0 :007 > dt = Date.today.send(:dt)
 => #<Java::OrgJodaTime::DateTime:0x3745e5c6> 
jruby-9.1.17.0 :008 > dt.to_s
 => "2019-01-29T00:00:00.000Z" 
jruby-9.1.17.0 :009 > dt.zone
 => #<Java::OrgJodaTime::UTCDateTimeZone:0x402e37bc> 
jruby-9.1.17.0 :010 > dt.chronology
 => #<Java::OrgJodaTimeChrono::GJChronology:0x22635ba0> 
@kares

This comment has been minimized.

Copy link
Member

kares commented Jan 29, 2019

My suggestion was to not use CHRONO_ITALY_UTC in the today implementation, but to have another constant (sayyy CHRONO_ITALY) that is initialized with GJChronology.getInstance() (EXACTLY that, with no arguments). You could probably just call that directly if another constant isn't desirable, but I think a constant would be more efficient.

fyi: does not take ENV['TZ'] into account ... but we can deal with that (@headius' original fix did that)

@kares

This comment has been minimized.

Copy link
Member

kares commented Jan 29, 2019

think I got how this should go and how JRuby might have regressed, we just need to interpret today properly but than store the internal DateTime as UTC ... just like it is atm. MRI seems to be doing just that :

2.5.3 :004 > Time.now.zone
 => "CET" 
2.5.3 :005 > Date.today.send(:zone)
 => "+00:00" 
2.5.3 :006 > DateTime.now.zone
 => "+01:00" 

kares added a commit to kares/jruby that referenced this issue Jan 29, 2019

[fix] interpret today TZ-relative but use GJ chrono
this should match the logic that was in place in 9.1.17.0
expected to, finally and properly, resolve jrubyGH-5418

@enebo enebo closed this in #5581 Jan 29, 2019

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.