Made time_class thread safe

[mmlac]

This should fix #189 and might close #182

[davispuh]

Seems good. 👍

[davispuh]

Probably Chronic.debug should also be per-thread.

[leejarvis]

I don't think this is necessary. I'd like to add the ability to use debug/time_class configuration values per Parser instance. That would solve the problem of hitting race conditions. Then Chronic.debug and Chronic.time_class can be used as 'global configuration', that is, default values which are overridden by a call to .parse:

Chronic.parse('next tuesday at 3pm', time_class: Time.zone)

I'm not too worried about the debug flag, though. If someone has enabled debugging they should expect to see it everywhere.

[davispuh]

Well I think thread safety is needed. To allow configuration, could create global_time_class which would be class variable thus global then there would be time_class which would return global_time_class if current thread's time_class isn't set. And then of course also possible set it as option to .parse

it could be like this

class << self
    def global_time_class
      @@global_time_class || ::Time
    end
    def global_time_class=(time)
      @@global_time_class = time
    end

    def time_class
      Thread.current[:__Chronic_time_class] || global_time_class
    end
    def time_class=(time)
      Thread.current[:__Chronic_time_class] = time
    end
end

then global_time_class would be on whole app while still allowing to set per-thread if have such need.

As for debug could do same, it's minor change and maybe for some cases it might be useful to debug only single thread.

[leejarvis]

Again I don't think this is necessary. If we are going to go the route of making stuff thread stuff like this, then it should be kept simple:

class Chronic
  def self.time_class
    Thread.current[:chronic_time_class] || ::Time
  end

  def self.time_class=(klass)
    Thread.current[:chronic_time_class] = klass
  end
end

There's no need for the prefixed underscores, or the capitalized class name, it's adding unnecessary complexity to what's essentially a simple subject.

However, I do still believe that we should allow setting the time class per Parser instance, and that having this ability means the global configuration does not have to be thread safe. If there's a per parser instance configuration value for time classes, there's absolutely no reason to mutate the global state, and it could be misleading to do so (for example having a Rails initializer with time_class set when someone could change it per thread)

[davispuh]

However, I do still believe that we should allow setting the time class per Parser instance

I'm not saying we shouldn't, it's a must have.

That global class variable would be just extra. But maybe not really needed then, I just like very customizable software so to fit everyone needs 😆

[adambrod]

++ thread safety

[gkop]

+1.

Also it may be worth looking at how Timecop handles this and whether Timecop is threadsafe. I like the Timecop API since it lets you run arbitrary code within the block. If we were to copy it for Chronic:

Chronic.time_class(Time.zone) do
  Chronic.parse('next tuesday at 3pm')
end

[pawurb]

+1 thread safety

[nzifnab]

Whatever came about this? Is Chronic.time_class thread safe in 2018, or not? :< We've been using it in production and I'm a bit concerned that we've been doing it wrong heh.