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

Time#zone using Time#local #1795

Open
robin850 opened this Issue Jul 6, 2014 · 6 comments

Comments

Projects
None yet
5 participants
@robin850
Contributor

robin850 commented Jul 6, 2014

Hello,

We've discovered an inconsistency of JRuby with MRI when dealing with time zones. The behavior is kind of weird on MRI anyway but we wanted with @febuiles to bring a discussion around this. Time.local supports ten parameters for historical reasons (when dealing with #to_a for instance) but the passed time zone is ignored and MRI and Rubinius returns the current time zone:

>> time = Time.local(0, 30, 1, 30, 10, 2005, 0, 0, false, 'US/Eastern');

>> RUBY_ENGINE
=> "ruby"
>> time.zone
=> "CEST" # In my case

>> RUBY_ENGINE
=> "rbx"
>> time.zone
=> "CEST"

However, JRuby has a sightly different behavior that isn't the same between JRuby 1.7.12 and 1.7.13:

>> JRUBY_VERSION
=> "1.7.12"
>> time.zone
=> nil

>> JRUBY_VERSION
=> "1.7.13"
>> time.zone
=> "EDT"

The returned time zone is wrong anyway because EDT stands for "Eastern Daylight Time" while false has been passed as the ninth parameter (i.e. isdst).

To bring JRuby in line with MRI and Rubinius, these lines should not be reached when we are calling Time#local.

Have a nice day!

@nielspetersen

This comment has been minimized.

Show comment
Hide comment
@nielspetersen

nielspetersen Feb 2, 2016

This issue is part of the current issue (14th) of the RubyIssues. https://rubyissues.ongoodbits.com/. 💎

nielspetersen commented Feb 2, 2016

This issue is part of the current issue (14th) of the RubyIssues. https://rubyissues.ongoodbits.com/. 💎

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 13, 2016

Member

This is still broken on current JRuby master:

$ jruby -ve "p Time.local(0, 30, 1, 30, 10, 2005, 0, 0, false, 'US/Eastern').zone"
jruby 9.1.0.0-SNAPSHOT (2.3.0) 2016-02-13 8136dd9 Java HotSpot(TM) 64-Bit Server VM 25.60-b23 on 1.8.0_60-b27 +jit [darwin-x86_64]
"EDT"
Member

headius commented Feb 13, 2016

This is still broken on current JRuby master:

$ jruby -ve "p Time.local(0, 30, 1, 30, 10, 2005, 0, 0, false, 'US/Eastern').zone"
jruby 9.1.0.0-SNAPSHOT (2.3.0) 2016-02-13 8136dd9 Java HotSpot(TM) 64-Bit Server VM 25.60-b23 on 1.8.0_60-b27 +jit [darwin-x86_64]
"EDT"
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 13, 2016

Member

Ok, I think I have a lead.

I think the problem here is that the given time and date are right at the DT/ST boundary for 2005 in that time zone.

In 2005, DST transition occured on October 30 at 2AM. You gave a time of 1:30AM, which for this case would require us to negotiated which side of the DST boundary we are on. I believe it's picking EDT because it sees the time is before 2AM, and therefore pre-transition.

We build up the time starting with all zeros and UTC and add in the month, day, hour, minute, etc, and then eventually apply the time zone. There's some logic related to the isDST parameter that appears to try to negotiate which side of the DT/ST boundary we are on, but passing true does not seem to affect anything. 1:30AM always comes out as EDT, and 2:30AM always comes out as EST.

I modified our code to use the isdst the same way MRI does, flopping the pre/post-transition flag, and I was able to get it to do EST and EDT predictably. Of course this isn't correct, since MRI just ignores the given time zone entirely.

And removing our time zone argument handling broke a bunch of other things.

Member

headius commented Feb 13, 2016

Ok, I think I have a lead.

I think the problem here is that the given time and date are right at the DT/ST boundary for 2005 in that time zone.

In 2005, DST transition occured on October 30 at 2AM. You gave a time of 1:30AM, which for this case would require us to negotiated which side of the DST boundary we are on. I believe it's picking EDT because it sees the time is before 2AM, and therefore pre-transition.

We build up the time starting with all zeros and UTC and add in the month, day, hour, minute, etc, and then eventually apply the time zone. There's some logic related to the isDST parameter that appears to try to negotiate which side of the DT/ST boundary we are on, but passing true does not seem to affect anything. 1:30AM always comes out as EDT, and 2:30AM always comes out as EST.

I modified our code to use the isdst the same way MRI does, flopping the pre/post-transition flag, and I was able to get it to do EST and EDT predictably. Of course this isn't correct, since MRI just ignores the given time zone entirely.

And removing our time zone argument handling broke a bunch of other things.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 13, 2016

Member

Here's the diff. The first part is removing our TZ arg processing, and the second part is using isdst like MRI does to decide the DT/ST boundary.

diff --git a/core/src/main/java/org/jruby/RubyTime.java b/core/src/main/java/org/jruby/RubyTime.java
index 9afc81c..8497fc7 100644
--- a/core/src/main/java/org/jruby/RubyTime.java
+++ b/core/src/main/java/org/jruby/RubyTime.java
@@ -1368,16 +1368,6 @@ public class RubyTime extends RubyObject {
         DateTimeZone dtz;
         if (gmt) {
             dtz = DateTimeZone.UTC;
-        } else if (args.length == 10 && args[9] instanceof RubyString) {
-            if (utcOffset) {
-                dtz = getTimeZoneFromUtcOffset(runtime, args[9]);
-                setTzRelative = true;
-            } else {
-                dtz = getTimeZoneFromString(runtime, args[9].toString());
-            }
-        } else if (args.length == 10 && args[9].respondsTo("to_int")) {
-            IRubyObject offsetInt = args[9].callMethod(runtime.getCurrentContext(), "to_int");
-            dtz = getTimeZone(runtime, ((RubyNumeric) offsetInt).getLongValue());
         } else {
             dtz = getLocalTimeZone(runtime);
         }
@@ -1485,15 +1475,17 @@ public class RubyTime extends RubyObject {
             dt = dt.withZoneRetainFields(dtz);

             // If we're at a DST boundary, we need to choose the correct side of the boundary
-            if (isDst) {
-                final DateTime beforeDstBoundary = dt.withEarlierOffsetAtOverlap();
-                final DateTime afterDstBoundary = dt.withLaterOffsetAtOverlap();
+            final DateTime beforeDstBoundary = dt.withEarlierOffsetAtOverlap();
+            final DateTime afterDstBoundary = dt.withLaterOffsetAtOverlap();

-                final int offsetBeforeBoundary = dtz.getOffset(beforeDstBoundary);
-                final int offsetAfterBoundary = dtz.getOffset(afterDstBoundary);
+            final int offsetBeforeBoundary = dtz.getOffset(beforeDstBoundary);
+            final int offsetAfterBoundary = dtz.getOffset(afterDstBoundary);

-                // If the time is during DST, we need to pick the time with the highest offset
+            // If the time is during DST, we need to pick the time with the highest offset
+            if (isDst) {
                 dt = offsetBeforeBoundary > offsetAfterBoundary ? beforeDstBoundary : afterDstBoundary;
+            } else {
+                dt = offsetBeforeBoundary > offsetAfterBoundary ? afterDstBoundary : beforeDstBoundary;
             }
         } catch (org.joda.time.IllegalFieldValueException e) {
             throw runtime.newArgumentError("time out of range");
Member

headius commented Feb 13, 2016

Here's the diff. The first part is removing our TZ arg processing, and the second part is using isdst like MRI does to decide the DT/ST boundary.

diff --git a/core/src/main/java/org/jruby/RubyTime.java b/core/src/main/java/org/jruby/RubyTime.java
index 9afc81c..8497fc7 100644
--- a/core/src/main/java/org/jruby/RubyTime.java
+++ b/core/src/main/java/org/jruby/RubyTime.java
@@ -1368,16 +1368,6 @@ public class RubyTime extends RubyObject {
         DateTimeZone dtz;
         if (gmt) {
             dtz = DateTimeZone.UTC;
-        } else if (args.length == 10 && args[9] instanceof RubyString) {
-            if (utcOffset) {
-                dtz = getTimeZoneFromUtcOffset(runtime, args[9]);
-                setTzRelative = true;
-            } else {
-                dtz = getTimeZoneFromString(runtime, args[9].toString());
-            }
-        } else if (args.length == 10 && args[9].respondsTo("to_int")) {
-            IRubyObject offsetInt = args[9].callMethod(runtime.getCurrentContext(), "to_int");
-            dtz = getTimeZone(runtime, ((RubyNumeric) offsetInt).getLongValue());
         } else {
             dtz = getLocalTimeZone(runtime);
         }
@@ -1485,15 +1475,17 @@ public class RubyTime extends RubyObject {
             dt = dt.withZoneRetainFields(dtz);

             // If we're at a DST boundary, we need to choose the correct side of the boundary
-            if (isDst) {
-                final DateTime beforeDstBoundary = dt.withEarlierOffsetAtOverlap();
-                final DateTime afterDstBoundary = dt.withLaterOffsetAtOverlap();
+            final DateTime beforeDstBoundary = dt.withEarlierOffsetAtOverlap();
+            final DateTime afterDstBoundary = dt.withLaterOffsetAtOverlap();

-                final int offsetBeforeBoundary = dtz.getOffset(beforeDstBoundary);
-                final int offsetAfterBoundary = dtz.getOffset(afterDstBoundary);
+            final int offsetBeforeBoundary = dtz.getOffset(beforeDstBoundary);
+            final int offsetAfterBoundary = dtz.getOffset(afterDstBoundary);

-                // If the time is during DST, we need to pick the time with the highest offset
+            // If the time is during DST, we need to pick the time with the highest offset
+            if (isDst) {
                 dt = offsetBeforeBoundary > offsetAfterBoundary ? beforeDstBoundary : afterDstBoundary;
+            } else {
+                dt = offsetBeforeBoundary > offsetAfterBoundary ? afterDstBoundary : beforeDstBoundary;
             }
         } catch (org.joda.time.IllegalFieldValueException e) {
             throw runtime.newArgumentError("time out of range");
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 17, 2016

Member

Revisited this to try to finish it for 9.1.3.0, but the proposed patch above breaks a number of specs. Back to the drawing board...and punting to 9.1.4.0.

Member

headius commented Aug 17, 2016

Revisited this to try to finish it for 9.1.3.0, but the proposed patch above breaks a number of specs. Back to the drawing board...and punting to 9.1.4.0.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius May 15, 2018

Member

No new reports on this as far as I can tell and it's edge-casey behavior not on roadmap at present. Detargeting.

Member

headius commented May 15, 2018

No new reports on this as far as I can tell and it's edge-casey behavior not on roadmap at present. Detargeting.

@headius headius removed this from the JRuby 9.2.0.0 milestone May 15, 2018

pcarlisle added a commit to pcarlisle/puppet that referenced this issue May 25, 2018

(PUP-1952) Fix schedule type with jruby
Due to jruby/jruby#1795 this was failing to
calculate overlaps correctly on jruby. We shouldn't be doing our own
date math like this anyway so I rewrote the `match?` function. This
incidentally fixes a bug with arrays of ranges if an early range spanned
midnight but did not match and a later range did, the check on the first
range would return out of the function with false and the second range
would never be checked.

pcarlisle added a commit to pcarlisle/puppet that referenced this issue Jun 1, 2018

(PUP-1952) Fix schedule type with jruby
Due to jruby/jruby#1795 this was failing to
calculate overlaps correctly on jruby. We shouldn't be doing our own
date math like this anyway so I rewrote the `match?` function. This
incidentally fixes a bug with arrays of ranges if an early range spanned
midnight but did not match and a later range did, the check on the first
range would return out of the function with false and the second range
would never be checked.

pcarlisle added a commit to pcarlisle/puppet that referenced this issue Jun 1, 2018

(PUP-1952) Fix schedule type with jruby
Due to jruby/jruby#1795 this was failing to
calculate overlaps correctly on jruby. We shouldn't be doing our own
date math like this anyway so I rewrote the `match?` function. This
incidentally fixes a bug with arrays of ranges if an early range spanned
midnight but did not match and a later range did, the check on the first
range would return out of the function with false and the second range
would never be checked.

pcarlisle added a commit to pcarlisle/puppet that referenced this issue Jun 4, 2018

(PUP-1952) Fix schedule type with jruby
Due to jruby/jruby#1795 this was failing to
calculate overlaps correctly on jruby. We shouldn't be doing our own
date math like this anyway so I rewrote the `match?` function. This
incidentally fixes a bug with arrays of ranges if an early range spanned
midnight but did not match and a later range did, the check on the first
range would return out of the function with false and the second range
would never be checked.

pcarlisle added a commit to pcarlisle/puppet that referenced this issue Jun 5, 2018

(PUP-1952) Fix schedule type with jruby
Due to jruby/jruby#1795 this was failing to
calculate overlaps correctly on jruby. We shouldn't be doing our own
date math like this anyway so I rewrote the `match?` function. This
incidentally fixes a bug with arrays of ranges if an early range spanned
midnight but did not match and a later range did, the check on the first
range would return out of the function with false and the second range
would never be checked.

pcarlisle added a commit to pcarlisle/puppet that referenced this issue Jun 5, 2018

(PUP-1952) Fix schedule type with jruby
Due to jruby/jruby#1795 this was failing to
calculate overlaps correctly on jruby. We shouldn't be doing our own
date math like this anyway so I rewrote the `match?` function. This
incidentally fixes a bug with arrays of ranges if an early range spanned
midnight but did not match and a later range did, the check on the first
range would return out of the function with false and the second range
would never be checked.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment