Changes to Time.zone are not reflected in Chronic parsing

[ghiculescu]

1.9.3p286 :036 > Time.zone
 => (GMT+10:00) Brisbane 
1.9.3p286 :037 > Time.zone.parse '9am'
 => Thu, 04 Apr 2013 09:00:00 EST +10:00 
1.9.3p286 :038 > Chronic.parse '9am'
 => Thu, 04 Apr 2013 09:00:00 EST +10:00 
1.9.3p286 :039 > Time.zone = 'Greenwich'
 => "Greenwich" 
1.9.3p286 :040 > Time.zone.parse '9am'
 => Thu, 04 Apr 2013 09:00:00 GMT +00:00 
1.9.3p286 :041 > Chronic.parse '9am'
 => Thu, 04 Apr 2013 09:00:00 EST +10:00 
1.9.3p286 :042 > Chronic.time_class
 => (GMT+10:00) Brisbane 

My rails app sets Time.zone on a user level, using an around_filter based on the current user. This causes subtle problems in Chronic parsing, which is why I've (reluctantly) stuck to Time.zone.parse so far.

The only solution I can think of is to Chronic.time_class on each request, after changing the Time.zone. But that makes things harder to test and doesn't feel right. I wonder if there is a better way of doing it?

[ghiculescu]

I tried the suggestion at https://github.com/mojombo/chronic/issues/158#issuecomment-11580360 but it didn't seem to make a difference (though it's not clear to me if any changes were actually made or if @injekt was just suggesting possible changes)

1.9.3p286 :045 > Chronic.parse('9am', time_class: Time.zone)
 => Thu, 04 Apr 2013 09:00:00 EST +10:00 

[leejarvis]

Yeah I was suggesting that as a change, it's not yet possible. I'll mark this as a feature.

[dnagir]

Is there a usable workaround currently?

All I can see to work this around is the following: (assuming time_class as an option for parse isn't supported yet)

def parse_with_tz(value)
  previous = Chronic.time_class
  begin
    Chronic.time_class = Time.zone
    return Chronic.parse(value)
  ensure
    Chronic.time_class = previous
  end
end

I was expecting this NOT to be thread-safe. However, I could not confirm it on MRI and Rubinius: https://gist.github.com/dnagir/5329defec514596fdc6e

So I'm a little puzzled as to whether there's a need in syncronisation (Mutex) or not.

[dnagir]

UPDATE: I've tested it on JRuby and it does indeed break there. So I can confirm that synchronisation is indeed required for the parse_with_tz. For example:

SYNC = Mutex.new

def parse_with_tz(value)
  SYNC.synchronize do
    previous = Chronic.time_class
    begin
      Chronic.time_class = Time.zone
      return Chronic.parse(value)
    ensure
      Chronic.time_class = previous
    end
  end
end

[dnagir]

UPDATE 2: All implementations will require synchronisation. The fact that MRI and Rubinius didn't in my has no conclusion yet. This is because the Ruby class vars are generally unsafe as proven by this, simplified test - https://gist.github.com/dnagir/80df45c96b49776dd174

[zxiest]

Maybe this will help someone: http://stackoverflow.com/a/15786663/226255

Chronic.parse("next 5:00 pm", 
:time_class => ActiveSupport::TimeZone.new(User.first.time_zone)).utc

[dnagir]

I don't know how that SO answer can help because time_class option is Not supported at all as far as I can see. On 25 Jun 2014 18:25, "Abdo" [email protected] wrote:

Maybe this will help someone: http://stackoverflow.com/a/15786663/226255

Chronic.parse("next 5:00 pm", :time_class => ActiveSupport::TimeZone.new(User.first.time_zone)).utc

— Reply to this email directly or view it on GitHub https://github.com/mojombo/chronic/issues/182#issuecomment-47073576.

[leejarvis]

It's correct that this is not thread-safe. There is currently no workaround until we build something into Chronic (see the other issues). This shouldn't be so hard now that parse creates a new Chronic::Parser instance. We an feed the time_class option into the Parser class and let it default back to Chronic.time_class. Probably once this option is added Chronic.time_class should be removed to avoid questioning thread safety (it'll never be thread-safe).

To clarify and reiterate what @dnagir has said; that SO answer is incorrect. I've commented on it.

[dnagir]

@leejarvis roger that.

I'll see if I can send a PR within a few days or so with all that sorted.

Unless you have it sorted already of course?

[leejarvis]

@dnagir Honestly, I don't. I haven't managed to find time for Chronic for quite a while. As frustrating as it is, I do plan to find some time to work on it (though I am working on a replacement, too). Work is very busy. Please feel free to submit a PR for it. @davispuh is currently managing a lot of the code also so one of us can review. Happy to expand collaborators. Still hoping to move the repo or sort something out so I don't have to bother @mojombo every time.

[davispuh]

I'm still waiting for PRs to be reviewed by @leejarvis some could be merged, but this isn't my lib so I'm waiting ;)

[dnagir]

Do you guys develop Chronic on Ruby 2?

I'm given error even when I try to bundle it:

$ bundle
There was a LoadError while loading chronic.gemspec: 
cannot load such file -- numerizer from
  /Users/dnagir/proj/chronic/chronic.gemspec:2:in `<main>'

Does it try to require a relative path? That's been removed in Ruby 1.9.

[davispuh]

it's because gemspec includes whole chronic and thus you get this exception because you don't have numerizer gem, it's fixed in #262 but currently you can just gem install numerizer

[dnagir]

Well, I just had a closer look at the parser.rb file.

This whole drama can be avoided easily by using now option.

This makes me wonder what's the point of the time_class option at all then? What should happen if both now and time_class options are given?

The solution to the whole issue is as simple as: Chronic.parse '9am', now: Time.zone.now

[dnagir]

In the meanwhile, this PR would make it clear about the time_class support: https://github.com/mojombo/chronic/pull/269

[dnagir]

Well, I see why now now options doesn't work (which it should) to fix this issue.

The time_class is being used all over the place:

lib/chronic/date.rb
66:      start_year = Chronic.time_class.now.year - bias

lib/chronic/handlers.rb
10:      day_start = Chronic.time_class.local(year, month, day)
78:        day_start = Chronic.time_class.local(year, month, day)
126:        end_time = Chronic.time_class.local(next_month_year, next_month_month)
127:        Span.new(Chronic.time_class.local(year, month), end_time)
135:      t = Chronic.time_class.parse(options[:text])
151:        day_start = Chronic.time_class.local(year, month, day)
168:        day_start = Chronic.time_class.local(year, month, day)
185:        day_start = Chronic.time_class.local(year, month, day)
209:        day_start = Chronic.time_class.local(year, month, day)
240:        day_start = Chronic.time_class.local(year, month, day)
241:        day_start = Chronic.time_class.local(year + 1, month, day) if options[:context] == :future && day_start < now
265:        end_time = Chronic.time_class.local(next_month_year, next_month_month)
266:        Span.new(Chronic.time_class.local(year, month), end_time)
297:          start_time = Chronic.time_class.local(year, month.index, day)
301:          day_start = Chronic.time_class.local(year, month.index, day)
318:        start_time = Chronic.time_class.local(year, month.index, day)
340:          start_time = Chronic.time_class.local(year, month, day)
344:          day_start = Chronic.time_class.local(year, month, day)
363:          start_time = Chronic.time_class.local(year, month.index, day)
367:          day_start = Chronic.time_class.local(year, month.index, day)
384:        start_time = Chronic.time_class.local(year, month.index, day)
399:        time = Chronic.time_class.local(year, month, day, h, m, s)
400:        end_time = Chronic.time_class.local(year, month, day + 1, h, m, s)
402:        time = Chronic.time_class.local(year, month, day)
404:        end_time = Chronic.time_class.local(year, month, day)
583:      Chronic.time_class.local(*date_parts)

lib/chronic/parser.rb
54:      @now = options.delete(:now) || Chronic.time_class.now

lib/chronic/repeaters/repeater_day.rb
14:        @current_day_start = Chronic.time_class.local(@now.year, @now.month, @now.day)

lib/chronic/repeaters/repeater_time.rb
79:        midnight = Chronic.time_class.local(@now.year, @now.month, @now.day)

lib/chronic/repeaters/repeater_week.rb
40:        this_week_start = Chronic.time_class.local(@now.year, @now.month, @now.day, @now.hour) + RepeaterHour::HOUR_SECONDS
47:        this_week_end = Chronic.time_class.local(@now.year, @now.month, @now.day, @now.hour)

lib/chronic/time.rb
31:      offset = Chronic.time_class.now.to_time.utc_offset unless offset # get current system's UTC offset if offset is nil

lib/chronic.rb
69:    #   Chronic.time_class = Time.zone
74:    attr_accessor :time_class
78:  self.time_class = ::Time
140:    if Chronic.time_class.name == "Date"
141:      Chronic.time_class.new(year, month, day)
142:    elsif not Chronic.time_class.respond_to?(:new) or (RUBY_VERSION.to_f < 1.9 and Chronic.time_class.name == "Time")
143:      Chronic.time_class.local(year, month, day, hour, minute, second)
145:      offset = Time::normalize_offset(offset) if Chronic.time_class.name == "DateTime"
146:      Chronic.time_class.new(year, month, day, hour, minute, second, offset)

README.md
152:>> Chronic.time_class = Time.zone

test/test_chronic.rb
133:    org = Chronic.time_class
135:      Chronic.time_class = ::Time
139:      Chronic.time_class = org
144:    org = Chronic.time_class
146:      Chronic.time_class = ::Date
149:      Chronic.time_class = org
154:    org = Chronic.time_class
156:      Chronic.time_class = ::DateTime
160:      Chronic.time_class = org
168:    org = Chronic.time_class
172:      Chronic.time_class = ::Time.zone
175:      Chronic.time_class = ::Time.zone
178:      Chronic.time_class = org

So whether we add it not it as an option, or whether we use now option or not... is irrelevant because the global Chronic.time_class is still being used everywhere.

I'm giving up on this :(

[davispuh]

now and time_class are different options. You specify now to tell Chronic at which time it should treat things like "Today", "Tomorrow" etc., but with time_class you specify which Time class you want output to be in, eg. Time or Date class

EDIT: So I think correct behavior would be that when parsing 'Today at 5pm' it would use same Timezone as used for now option for parsing/input, but output would be in Timezone which is for time_class

[bjfish]

Would there be any issue with doing the following in rails?

             Time.use_zone('Pacific Time (US & Canada)') do
                 Chronic.time_class = Time.zone
                 Chronic.parse("Sunday")
             end

Thank you

[chewi]

As @kbaum pointed out in the other issue, Time.zone= is thread local. What @bjfish proposed above is still not safe as Chronic.time_class may get called by other threads during that block, however there is one easy workaround. Stick this in an initializer.

module Chronic
  def self.time_class
    ::Time.zone
  end
end

Then you can safely do this.

Time.use_zone('Pacific Time (US & Canada)') do
  Chronic.parse("Sunday")
end

[espen]

That looks like a good workaround @chewi ,have you been using this in production?

[chewi]

Unfortunately I've completely forgotten why I looked into this. I don't believe we're using that workaround in production at the moment.

[mphalliday]

@espen we've been using @chewi's workaround for a few weeks in production now. Working well for us.

[espen]

great @mphalliday, thanks for the feedback. My tests so far looks good. Thanks for sharing @chewi