v0.4.0+ breaks one-time configuration (evidenced in validates_timeliness)

[timdiggins]

v0.4.0 which brought in thread safety (#21, #23) seems to have broken configurability of some servers - I'm working with puma, but I think others may exhibit this functionality.

I think this is because the changes in #23 require each new thread to be configured, whereas the rails initializers are assuming a copy-on-write approach.

Here's a spec to demonstrate (I have higher level (system) specs that demonstrate the real world problem, but they are bound into the codebase):

require "rails_helper"

RSpec.describe Timeliness do

  after(:each) do
    # reset to not mess up other specs
    load(Rails.root.join("config/initializers/validates_timeliness.rb"))
  end

  let(:us_date) { "06/30/2016" }
  let(:eu_date) { "30/06/2016" }

  it "is thread_safe (fails with Timeliness < 0.4, fixed with Timeliness >= 0.4)" do
    threads = []
    threads << Thread.new do
      Timeliness.use_euro_formats
      10_000.times { expect(Timeliness.parse(eu_date)).not_to be_nil }
    end
    threads << Thread.new do
      Timeliness.use_us_formats
      10_000.times { expect(Timeliness.parse(us_date)).not_to be_nil }
    end
    threads.each(&:join)
  end

  it "doesn't need re-configuration per thread (fails with Timeliness >= 0.4, fixed with Timeliness <= 0.4)" do
    Timeliness.use_euro_formats
    expect(Timeliness.parse(eu_date)).not_to be_nil
    expect(Timeliness.parse(us_date)).to be_nil
    threads = []
    threads << Thread.new { expect(Timeliness.parse(eu_date)).not_to be_nil }
    threads << Thread.new { expect(Timeliness.parse(us_date)).to be_nil }
    threads.each(&:join)
  end
end

fyi, config/initializers/validates_timeliness.rb:

ValidatesTimeliness.setup do |config|
  # ....

  # Treat ambiguous dates, such as 01/02/1950, as a Non-US date.
  config.parser.remove_us_formats
end

[adzap]

Yes, unfortunately the threadsafe switching forced a compromise. In v0.4.0 the format set was empty in each new thread, which forced you to use remove_us_formats (or better use_euro_formats) to initialise the parser.

In v0.4.2 I fixed it so that in new threads the format set would initially be the main thread's format set, but required the use of a new config setting i.e. Timeliness.ambiguous_date_format = :euro. Since we need to initialise the format set to something for each new thread, we need a default. But we should also allow the developer to control the default. In the past this was simple in the non-threadsafe version, just Timeliness.use_euro_formats. In threadsafe mode we need another way because we can't use the same method that switches formats to also set the default when we don't know what context the gem is being used e.g. a booting Rails app or something else, and it would require us to use mutex locking and would alter the default for each new thread which we don't want.

So I added a config option that can put the onus on the developer to set it in a threadsafe way i.e. before spawning new threads, or your case during Rails boot. But as you've pointed out for existing use in ValidatesTimeliness this is a problem because if you just update to the new Timeliness the way you used to set the default doesn't work.

Because ValidatesTimeliness has a way of knowing its context and app state i.e. the Railtie, I think any change to handle this issue needs to be done in that gem's railtie not in this gem.

Overall I'm not really happy with the gem's management of format sets and I'm looking into changing it for a v1.0 release.

[adzap]

@timdiggins I've pushed a change to ValidatesTimeliness on 4-0-stable branch if you want to try out my solution.

[adzap]

see latest ValidatesTimeliness which I think should fix your issue.

[timdiggins]

hi @adzap I've tried out the change but my test is failing. So have continued discussion in https://github.com/adzap/validates_timeliness/issues/187

[timdiggins]

PS -- sorry it took sooooo long for me to get back to you on this!