Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Mention that Timeliness is not thread safe. #21

Closed
andruby opened this issue Jul 1, 2016 · 5 comments · Fixed by #27
Closed

Mention that Timeliness is not thread safe. #21

andruby opened this issue Jul 1, 2016 · 5 comments · Fixed by #27

Comments

@andruby
Copy link
Contributor

andruby commented Jul 1, 2016

Reproducable with:

eu_date = "30/06/2016"
us_date = "06/30/2016"
threads = []
threads << Thread.new { Timeliness.use_euro_formats; 10_000.times { Timeliness.parse(eu_date).to_date } }
threads << Thread.new { Timeliness.use_us_formats; 10_000.times { Timeliness.parse(us_date).to_date } }
threads.each { |thr| thr.join }

This will raise a NoMethodError: undefined method 'to_date' for nil:NilClass because it parsed one of the dates in the wrong US/EU format.

I suggest we at least mention this in the README. We bumped into this using Timeliness inside sidekiq workers.

@adzap
Copy link
Owner

adzap commented Jul 2, 2016

Ah yes fair enough. The gem is poorly designed for this usage. Though I am keen to resolve this. It shouldn't be too hard.

@andruby
Copy link
Contributor Author

andruby commented Jul 2, 2016

I'll take a shot at making it threadsafe on Monday.

I was thinking to store the class variables from Timeliness::Definitions in Thread.current[].

Is that the same approach you were considering?

On 02 Jul 2016, at 03:54, Adam Meehan notifications@github.com wrote:

Ah yes fair enough. The gem is poorly designed for this usage. Though I am keen to resolve this. It shouldn't be too hard.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@dgilperez
Copy link

Any news? Thanks!

@andruby
Copy link
Contributor Author

andruby commented Sep 11, 2016

Hi @dgilperez. I thought I had already opened a PR, but it doesn't seem that way. Here's the branch we use in production with sidekiq threads: https://github.com/andruby/timeliness/tree/multithread. I'll try to open a PR when I have time.

@lawso017
Copy link

lawso017 commented Feb 3, 2017

@adzap would you have a moment to merge @andruby 's multithread changes into Timeliness? Looking forward to the ability to adjust US/ROW date parsing on a per-request basis, already using your excellent validate gem but would need an updaetd timeliness gem with thread local configuration info.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants