validates_timeliness icon indicating copy to clipboard operation
validates_timeliness copied to clipboard

No configuration required per thread (compatibility with timeliness 0.4.0+

Open timdiggins opened this issue 4 years ago • 7 comments

Following on from https://github.com/adzap/timeliness/issues/37...

I'm not seeing a fix for this in my spec (see below) for pre-threaded configuration / threadsafety.

Results:


timeliness validates_timeliness no reconfiguration per thread thread-safety
v0.3.10 v4.0.10 passes fails
v0.4.3 v4.0.10 fails passes
v0.4.3 v5.0.0.alpha5 fails passes

My spec for this is as follows:

# frozen_string_literal: true

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 "doesn't need re-configuration per thread (fails with Timeliness >= 0.4 but should be fixed with ValidatesTimeliness >= 5.0.0.alpha5)" do
    # Timeliness.use_euro_formats -- in initializer
    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

  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 do |t|
      t.report_on_exception = false
      t.join
    end
  end
end

timdiggins avatar Aug 31 '19 06:08 timdiggins

Thanks @timdiggins . Given you are loading the initializer explicitly I wonder if you are actually booting rails for these specs such the railties are loaded?

The necessary step happens in the Railtie here https://github.com/adzap/validates_timeliness/blob/3835b2b1619d99fceaa2c34dda6603d9985a3bfd/lib/validates_timeliness/railtie.rb#L15

adzap avatar Aug 31 '19 18:08 adzap

Hi @adzap The (re)loading of the initializer only happens after the specs-- it's to stop the specs from messing up with any others (in my full suite). I can remove it and it still runs...

These lines prove the initializer is running:

    expect(Timeliness.parse(eu_date)).not_to be_nil
    expect(Timeliness.parse(us_date)).to be_nil

The spec fails during Thread.join.

I've put some debugging lines into my initializer and also the railtie (within validates_timeliness.initialize_timeliness_ambiguous_date_format) e.g.:

p(at: "config/initializers/validates_timeliness.rb (end)", current_date_format: Timeliness::Definitions.current_date_format, ambiguous_date_format: Timeliness.configuration.ambiguous_date_format)

The output indicates that the railtie is running before. (This happens both in the specs, but also when I run rails s)

{:at=>"in validates_timeliness.initialize_timeliness_ambiguous_date_format (before)", :current_date_format=>:us, :ambiguous_date_format=>:us}
{:at=>"in validates_timeliness.initialize_timeliness_ambiguous_date_format (after)", :current_date_format=>:us, :ambiguous_date_format=>:us}
{:at=>"config/initializers/validates_timeliness.rb (start)", :current_date_format=>:us, :ambiguous_date_format=>:us}
{:at=>"config/initializers/validates_timeliness.rb (end)", :current_date_format=>:euro, :ambiguous_date_format=>:us}

So is it possible that "engines_blank_point" is a better spot for hooking into than "load_config_initializers" (maybe the .after is not being respected?) https://guides.rubyonrails.org/v5.2/configuring.html

timdiggins avatar Sep 02 '19 11:09 timdiggins

Hmmm, weird. Even engines_blank_point is giving me the same ordered output. I can get the correct ordering only by changing it from an initializer block to a config.to_prepare block.

Maybe I should check that I can reproduce this in a vanilla rails project. Will share with you if I can (but won't be for a few days)

timdiggins avatar Sep 02 '19 12:09 timdiggins

@timdiggins how did you go with this? was it confirmed in vanilla Rails?

adzap avatar Jul 02 '20 01:07 adzap

@adzap I hadn't tried this out in vanilla rails, but I have now:

https://github.com/timdiggins/validates-timeliness-issue-187/commit/8d35a600495586421b83d417ecb8834e76789862

Still failing -- is there some flaw in my spec or config?

timdiggins avatar Jul 02 '20 06:07 timdiggins

Do you mind trying again against master? I changed the 'load_config_initializers' to symbol. I think I've run into this in the past. I will add a spec for this later.

adzap avatar Jul 04 '20 07:07 adzap

@adzap yes looks like master fixes this.

https://github.com/timdiggins/validates-timeliness-issue-187/blob/master/spec/lib/validates_timeliness_spec.rb

timdiggins avatar Jul 10 '20 15:07 timdiggins