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

Fully initialize TZInfo::TimezoneTransitionDefinition object before freeze #80

Closed
wants to merge 1 commit into
base: 1.2
from

Conversation

Projects
None yet
2 participants
@alsor

alsor commented Jan 26, 2018

TZInfo::Timezone objects could potentially be deep frozen before internal instances of TZInfo::TimezoneTransitionDefinition (which they may contain) will be fully initialized. If this happens then invoking at method will raise RuntimeError: can't modify frozen TZInfo::TimezoneTransitionDefinition exception (because of lazily computed @at instance variable).

Situation can be complicated with use of ActiveSupport::TimeWithZone instances (in Ruby on Rails projects for example) which can share cached TZInfo::Timezone instances internally - so deep freezing any ActiveSupport::TimeWithZone instance in one place of the code can cause mentioned exceptions to be raised in other, unrelated places.

The solution in this PR is similar to activesupport gem code, see ActiveSupport::TimeWithZone#freeze for example.

@philr

This comment has been minimized.

Member

philr commented Jan 27, 2018

Thank you for submitting this.

Forcing at to be evaluated when a TimezoneTransitionDefinition is frozen comes with a performance overhead (time to freeze and ongoing memory use) when multiplied by the large numbers of TimezoneTransitionDefinition instances that make up a Timezone. Most of the results will also not be used (due to most programs only being interested in a relatively small window of time). I think it is therefore better to disable storing the result of the lazy evaluation if the object has been frozen.

There are also other methods that make use of lazy evaluation, for example, local_start_at and local_end_at on TimezoneTransition (the base class of TimezoneTransitionDefinition) and to_time, to_datetime and to_i on TimeOrDateTime (the result type of TimezoneTransitionDefinition#at). These also need to be addressed in order that a frozen Timezone can be used.

I've just committed a change to the 1.2 branch (3e5684b) that handles freezing by disabling the storing of lazy evaluation results in all the places that are affected.

I've also committed a change to the master branch (939d306). The code on master is largely unaffected because most of the lazy evaluation has already been removed.

@philr philr closed this Jan 27, 2018

@alsor

This comment has been minimized.

alsor commented Jan 28, 2018

@philr Thank you very much!

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