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

No configuration required per thread (compatibility with timeliness 0.4.0+ #187

Open
timdiggins opened this issue Aug 31, 2019 · 7 comments

Comments

@timdiggins
Copy link
Contributor

Following on from adzap/timeliness#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
@adzap
Copy link
Owner

adzap commented Aug 31, 2019

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

initializer "validates_timeliness.initialize_timeliness_ambiguous_date_format", :after => 'load_config_initializers' do

@timdiggins
Copy link
Contributor Author

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
Copy link
Contributor Author

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)

@adzap
Copy link
Owner

adzap commented Jul 2, 2020

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

@timdiggins
Copy link
Contributor Author

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

timdiggins/validates-timeliness-issue-187@8d35a60

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

@adzap
Copy link
Owner

adzap commented Jul 4, 2020

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.

timdiggins added a commit to timdiggins/validates_timeliness that referenced this issue Jul 10, 2020
@timdiggins
Copy link
Contributor Author

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

No branches or pull requests

2 participants