chronic
chronic copied to clipboard
Made time_class thread safe
This should fix #189 and might close #182
Seems good. :+1:
Probably Chronic.debug
should also be per-thread.
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.
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.
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)
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 :laughing:
++ thread safety
+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
+1 thread safety
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.