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

Allow Time#to_time on frozen objects. Return frozen time rather than "RuntimeError: can't modify frozen Time" #28147

Merged
merged 1 commit into from Mar 16, 2017

Conversation

Projects
None yet
4 participants
@kmcphillips
Contributor

kmcphillips commented Feb 24, 2017

Problem

In 29f211f memoization was added to DateAndTime::Compatibility#to_time for performance reasons. Unfortunately, since Time can be frozen memozation is not possible. Any creating of ivars from inside a frozen object will immediately raise a RuntimeError. So any call to to_time on a frozen Time raises. This affects all stable versions of rails.

Solution

I have reverted the commit that adds this memoization. I added regression tests to cover the cases with frozen Time and DateTime objects, since the module is included in both.

In working through this problem and attempting to test it, I found that calling #to_time on a time does not return a frozen time, which is inconsistent:

> {}.freeze.to_hash.frozen?
=> true
> "".freeze.to_s.frozen?
=> true
> :x.freeze.to_sym.frozen?
=> true
> DateTime.new.freeze.to_datetime.frozen?
=> true

I have fixed this by returning a frozen time if the original was frozen.

Next steps

This affects 5-0-stable and 4-2-stable so I intend to backport the memoization revert and regression tests to those branches.

Question
Should I also backport the last commit making .freeze.to_time return a frozen time? It's a behaviour change.

There is actually a more serious regression specifically and exclusively in 4.2.8 that causes a RuntimeError: can't modify frozen Time to be raised whenever anything is compared with a frozen Time. Details in a follow up PR.

@rafaelfranca @pixeltrix

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Feb 24, 2017

Is there a reason we can't override freeze and call to_time inside DateAndTIme::Compatibility like we do for ActiveSupport::TimeWithZone ?

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Feb 24, 2017

Is there a reason we can't override freeze and call to_time inside DateAndTIme::Compatibility like we do for ActiveSupport::TimeWithZone ?

To answer my own question - it leaves the return Time instance as unfrozen and even doing this dance doesn't work:

    def freeze_with_compatibiliy #:nodoc:
      to_time.freeze_without_compatibiliy
      freeze_without_compatibiliy
    end

    alias_method :freeze_without_compatibiliy, :freeze
    alias_method :freeze, :freeze_with_compatibiliy

Because it leaves the return from to_time frozen and not cached.

It was a heck of a slow down so I'm loathe to revert that without giving it some thought.

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Feb 24, 2017

Hmm, not sure about the whole return the response of to_* as frozen - the examples you've given are because those methods are returning self and not because they're checking for an object is frozen. I think I read the 2.4 changes to to_time incorrect - it doesn't preserve the timezone, it just returns self instead. We should probably change the compatibility module to match this behavior.

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Feb 24, 2017

Using this script I checked the behaviour across all the Ruby versions this needs to work against:

Time#to_time              | 1.9.3 | 2.0.0 | 2.1.6 | 2.2.6 | 2.3.3 | 2.4.0 | 2.5.0 
--------------------------|-------|-------|-------|-------|-------|-------|-------
Maintains UTC offset      | f     | f     | f     | f     | f     | t     | t     
Converts to +00:00        | f     | f     | f     | f     | f     | f     | f     
Converts to TZ            | t     | t     | t     | t     | t     | f     | f     
Returns self              | f     | f     | f     | f     | f     | t     | t     
Returns frozen            | f     | f     | f     | f     | f     | t     | t     

Time#to_datetime          | 1.9.3 | 2.0.0 | 2.1.6 | 2.2.6 | 2.3.3 | 2.4.0 | 2.5.0 
--------------------------|-------|-------|-------|-------|-------|-------|-------
Maintains UTC offset      | t     | t     | t     | t     | t     | t     | t     
Converts to +00:00        | f     | f     | f     | f     | f     | f     | f     
Converts to TZ            | f     | f     | f     | f     | f     | f     | f     
Returns self              | f     | f     | f     | f     | f     | f     | f     
Returns frozen            | f     | f     | f     | f     | f     | f     | f     

Time#to_date              | 1.9.3 | 2.0.0 | 2.1.6 | 2.2.6 | 2.3.3 | 2.4.0 | 2.5.0 
--------------------------|-------|-------|-------|-------|-------|-------|-------
Returns self              | f     | f     | f     | f     | f     | f     | f     
Returns frozen            | f     | f     | f     | f     | f     | f     | f     

Date#to_time              | 1.9.3 | 2.0.0 | 2.1.6 | 2.2.6 | 2.3.3 | 2.4.0 | 2.5.0 
--------------------------|-------|-------|-------|-------|-------|-------|-------
Maintains UTC offset      | N/A   | N/A   | N/A   | N/A   | N/A   | N/A   | N/A    
Converts to +00:00        | f     | f     | f     | f     | f     | f     | f     
Converts to TZ            | t     | t     | t     | t     | t     | t     | t     
Returns self              | f     | f     | f     | f     | f     | f     | f     
Returns frozen            | f     | f     | f     | f     | f     | f     | f     

Date#to_datetime          | 1.9.3 | 2.0.0 | 2.1.6 | 2.2.6 | 2.3.3 | 2.4.0 | 2.5.0 
--------------------------|-------|-------|-------|-------|-------|-------|-------
Maintains UTC offset      | N/A   | N/A   | N/A   | N/A   | N/A   | N/A   | N/A    
Converts to +00:00        | t     | t     | t     | t     | t     | t     | t     
Converts to TZ            | f     | f     | f     | f     | f     | f     | f     
Returns self              | f     | f     | f     | f     | f     | f     | f     
Returns frozen            | f     | f     | f     | f     | f     | f     | f     

Date#to_date              | 1.9.3 | 2.0.0 | 2.1.6 | 2.2.6 | 2.3.3 | 2.4.0 | 2.5.0 
--------------------------|-------|-------|-------|-------|-------|-------|-------
Returns self              | t     | t     | t     | t     | t     | t     | t     
Returns frozen            | t     | t     | t     | t     | t     | t     | t     

DateTime#to_time          | 1.9.3 | 2.0.0 | 2.1.6 | 2.2.6 | 2.3.3 | 2.4.0 | 2.5.0 
--------------------------|-------|-------|-------|-------|-------|-------|-------
Maintains UTC offset      | f     | f     | f     | f     | f     | t     | t     
Converts to +00:00        | f     | f     | f     | f     | f     | f     | f     
Converts to TZ            | t     | t     | t     | t     | t     | f     | f     
Returns self              | f     | f     | f     | f     | f     | f     | f     
Returns frozen            | f     | f     | f     | f     | f     | f     | f     

DateTime#to_datetime      | 1.9.3 | 2.0.0 | 2.1.6 | 2.2.6 | 2.3.3 | 2.4.0 | 2.5.0 
--------------------------|-------|-------|-------|-------|-------|-------|-------
Maintains UTC offset      | t     | t     | t     | t     | t     | t     | t     
Converts to +00:00        | f     | f     | f     | f     | f     | f     | f     
Converts to TZ            | f     | f     | f     | f     | f     | f     | f     
Returns self              | t     | t     | t     | t     | t     | t     | t     
Returns frozen            | t     | t     | t     | t     | t     | t     | t     

DateTime#to_date          | 1.9.3 | 2.0.0 | 2.1.6 | 2.2.6 | 2.3.3 | 2.4.0 | 2.5.0 
--------------------------|-------|-------|-------|-------|-------|-------|-------
Returns self              | f     | f     | f     | f     | f     | f     | f     
Returns frozen            | f     | f     | f     | f     | f     | f     | f     

As you can see I think the frozen issue is a red herring - we just need to make sure that we return self and that will fix the problem. Given that I think it should be possible to cache the to_time before freezing like we do for ActiveSupport::TimeWithZone.

@kmcphillips

This comment has been minimized.

Contributor

kmcphillips commented Feb 24, 2017

Ok. Thanks for digging into this. I'll look at properly returning self right now.

I could conditionally memoize if it's not frozen, but that's a really nasty surprise for someone tracking perf later that the frozen record is actually slower.

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Feb 25, 2017

@kmcphillips if you've not done anything yet then I have a solution which does the following:

  1. Remove the to_time method from DateAndTime::Compatibility

  2. Add the following to_time implementation to active_support/core_ext/time/compatibility.rb:

    remove_possible_method :to_time
    
    def to_time
      preserve_timezone ? self : getlocal
    end
  3. Add the following to_time implementation to active_support/core_ext/date_time/compatibility.rb:

    remove_possible_method :to_time
    
    def to_time
      preserve_timezone ? getlocal(utc_offset) : getlocal
    end
  4. Add the following to_time implementation to active_support/time_with_zone.rb:

    def to_time
      if preserve_timezone
        @to_time_with_instance_offset ||= getlocal(utc_offset)
      else
        @to_time_with_system_offset ||= getlocal
      end
    end
  5. Add to_time to ActiveSupport::TimeWithZone#freeze, e.g:

    def freeze
      # preload instance variables before freezing
      period; utc; time; to_time; to_datetime
      super
    end
  6. Updated the tests as per this gist: https://gist.github.com/pixeltrix/00c3b8f2ab7d4fffe13de52bd5831a45

This solution returns self where appropriate and still fixes the previous performance issue which was around comparing two ActiveSupport::TimeWithZone instances. It also doesn't create any instance variables in core classes and only overrides the methods we were previously so should be less invasive.

There may be additional performance gains by rewriting ActiveSupport::TimeWithZone#<=> to check if the other class is an ActiveSupport::TimeWithZone instance and comparing the utc attributes directly but I haven't checked that.

@matthewd @rafaelfranca wdyt about this solution?

@kmcphillips

This comment has been minimized.

Contributor

kmcphillips commented Feb 25, 2017

@pixeltrix This is wonderful.

Yeah, you got further than me here. But doing to_time in freeze is the same direction I was going. And it lines up with how TimeWithZone is implemented. And the returning self seems like the correct solution.

Part of what exposed this bug in 4.2.8 is that time/calculations.rb on 4-2-stable is missing:

    if other.class == Time
      compare_without_coercion(other)

And always falls back to other.to_time. So in most cases it was passing self through before to_time was even called. So this all lines up. 😄

I was looking at how to essentially turn those tables above into tests to help prevent regressions.

I'd be happy to put this together this weekend and get tests covering all the cases, unless you're attached to doing it yourself. I'd be glad for the experience and to get a better understanding of date/time/zones. Always bugs to fix related to time in our apps.

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Feb 25, 2017

@kmcphillips happy for you to complete your PR - knock yourself out 😄

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Mar 3, 2017

@kmcphillips are you planning to work on this - if not I can pick it up

@kmcphillips

This comment has been minimized.

Contributor

kmcphillips commented Mar 3, 2017

@kmcphillips kmcphillips force-pushed the kmcphillips:master-time-freeze branch 3 times, most recently Mar 6, 2017

@kmcphillips kmcphillips force-pushed the kmcphillips:master-time-freeze branch to 92fc8ec Mar 6, 2017

@kmcphillips

This comment has been minimized.

Contributor

kmcphillips commented Mar 6, 2017

Ok! Thank you for your patience.

I've rewritten this. It mostly follows what you laid out @pixeltrix, pushing the memoziation deeper in and making to_time smarter. Plus more tests. I'll comment inline. There are one or two things I'm not sure about.

else
@_to_time_with_system_offset ||= getlocal
end
preserve_timezone ? getlocal(utc_offset) : getlocal

This comment has been minimized.

@kmcphillips

kmcphillips Mar 6, 2017

Contributor

This change fixes the original bug.

class Time
prepend DateAndTime::Compatibility
include DateAndTime::Compatibility

This comment has been minimized.

@kmcphillips

kmcphillips Mar 6, 2017

Contributor

Changes prepend to include. Not sure about this. Really it only is including the preserve_timezone property and the to_time we want to override. When it was prepending it would never reach the to_time in this file. Open to feedback.

assert_equal Time, @twz.to_time.class
assert_equal Time.local(1999, 12, 31, 19), @twz.to_time
assert_equal Time.local(1999, 12, 31, 19).utc_offset, @twz.to_time.utc_offset
def test_to_time_with_preserve_timezone

This comment has been minimized.

@kmcphillips

kmcphillips Mar 6, 2017

Contributor

These tests now check the preserve_timezone which is included into TWZ as part of DateAndTime::Compatibility included at the top.

@@ -508,6 +526,7 @@ def test_freeze_preloads_instance_variables
@twz.period
@twz.time
@twz.to_datetime
@twz.to_time

This comment has been minimized.

@kmcphillips

kmcphillips Mar 6, 2017

Contributor

Further tests the memoization and regression against the freezing bug.

prepend DateAndTime::Compatibility
include DateAndTime::Compatibility
remove_possible_method :to_time

This comment has been minimized.

@kmcphillips

kmcphillips Mar 6, 2017

Contributor

I'm fuzzy on the benefits of overshadowing vs undef_method. So this may not be necessary.

@kmcphillips

This comment has been minimized.

Contributor

kmcphillips commented Mar 9, 2017

Gentle nudge for reviews. 🙃

@pixeltrix pixeltrix merged commit ee33b9e into rails:master Mar 16, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Mar 16, 2017

@kmcphillips thanks! 👍

@kmcphillips

This comment has been minimized.

Contributor

kmcphillips commented Mar 16, 2017

Great! I'll work on backporting today.

kmcphillips added a commit to kmcphillips/rails that referenced this pull request Mar 16, 2017

Merge pull request rails#28147 from kmcphillips/master-time-freeze
Allow Time#to_time on frozen objects. Return frozen time rather than "RuntimeError: can't modify frozen Time"

kmcphillips added a commit to kmcphillips/rails that referenced this pull request Mar 17, 2017

Merge pull request rails#28147 from kmcphillips/master-time-freeze
Allow Time#to_time on frozen objects. Return frozen time rather than "RuntimeError: can't modify frozen Time"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment