Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Thread safety #1700

Closed
brenttheisen opened this issue Aug 21, 2019 · 9 comments
Closed

Thread safety #1700

brenttheisen opened this issue Aug 21, 2019 · 9 comments
Labels
🏷️ Issue: Has Attached PR 🪜Issue: With Reproduction Steps ↩ Needs Revisiting This issue or PR has been brought back and needs to be worked on.

Comments

@brenttheisen
Copy link

It appears as though every method defined in this gem is intended to be called as a class method, not an instance method. Of course these class methods often modify class variables which makes it impossible to use this gem in a thread safe way without locking threads.

Was the decision made to sacrifice thread safety in order to call methods easily on classes? Were there any other reasons why thread safety wasn't considered?

I'd like to use this gem to generate deterministic fake data for a preproduction API that runs in Puma (threads instead of processes). Was hoping there was a way to use it without needing to lock.

@Zeragamba
Copy link
Contributor

I don't think there are any places that modify values and everything should be deterministic. Can you point out some example on where the thread safety breaks down?

@brenttheisen
Copy link
Author

brenttheisen commented Aug 22, 2019

@Zeragamba: I need to call Faker::Config.random= to set an instance of Random that is seeded with different values in different threads. That method is a class method that alters a class variable so there is no way to do this in a thread safe way. Is there such a thing as instances of the Faker::* classes?

Here's a reproduction script...

expected_values = 2.times.map do |index|
  Faker::Config.random = Random.new(index)
  Faker::Number.digit
end

threads = expected_values.each_with_index.map do |expected_value, index|
  Thread.new do
    100000.times.each do |run_count|
      Faker::Config.random = Random.new(index)
      output = Faker::Number.digit

      if output != expected_value
        raise "Thread #{index} run occurrence #{run_count} expected #{expected_value} " +
          "but got #{output}"
      end
    end

    puts "Thread #{index} completed successfully"
  end
end

threads.join

And here's a run where it reproduced...

2.6.3-p62: 001> load './faker_thread_safe_error.rb'
=> true
2.6.3-p62: 002> #<Thread:0x00007fb4ee197eb8@./faker_thread_safe_error.rb:7 run> terminated with exception (report_on_exception is true):
Traceback (most recent call last):
        3: from ./faker_thread_safe_error.rb:8:in `block (2 levels) in <top (required)>'
        2: from ./faker_thread_safe_error.rb:8:in `each'
        1: from ./faker_thread_safe_error.rb:8:in `times'
./faker_thread_safe_error.rb:13:in `block (3 levels) in <top (required)>': Thread 1 run occurrence 35319 expected 5 but got 0 (RuntimeError)
Thread 0 completed successfully

@project-eutopia
Copy link

Just wanted to check in and see if any progress has been made on this issue. I also would like to be able to instantiate a Faker instance with its own random object, so that the thread which owns this instance can have deterministic randomized output no matter what other threads may be doing.

@wilg
Copy link

wilg commented Jul 21, 2021

Agree this is a big issue for me as well. Any pointers on how this change might be able to be made? Maybe just an option for Faker::Config.thread_local_random or something

@wilg
Copy link

wilg commented Jul 21, 2021

I think probably for the same reasons unique isn't thread safe

@thdaraujo
Copy link
Contributor

Hey, folks. In an effort to lighten our load as maintainers and be able to serve you better in the future, the faker-ruby team is working on cleaning out the cobwebs in this repo by pruning the backlog. As there are few of us, there are a lot of items that will simply never earn our attention in a reasonable time frame, and rather than giving you an empty promise, we think it makes more sense to focus on more recent issues. That means, unfortunately, that we must close this issue.

Don't take this the wrong way: our aim is not to diminish the effort people have made or dismiss problems that have been raised. If you feel that we should reopen this issue, then please let us know so that we can reprioritize it. Thanks!

@kiskoza kiskoza mentioned this issue Aug 8, 2022
@thdaraujo thdaraujo added 🪜Issue: With Reproduction Steps ↩ Needs Revisiting This issue or PR has been brought back and needs to be worked on. and removed ❓ Question labels Aug 13, 2022
@thdaraujo
Copy link
Contributor

thdaraujo commented Aug 13, 2022

I was able to reproduce the issue based on this suggestion

require 'faker'

expected_values = 2.times.map do |index|
  Faker::Config.random = Random.new(index)
  Faker::Number.digit
end

threads = expected_values.each_with_index.map do |expected_value, index|
  Thread.new do
    1_000_000.times.each do |run_count|
      Faker::Config.random = Random.new(index)
      output = Faker::Number.digit

      if output != expected_value
        raise "Thread #{index} run occurrence #{run_count} expected #{expected_value} " +
                "but got #{output}"
      end
    end

    puts "Thread #{index} completed successfully"
  end
end

threads.each(&:join)

Result:

Using faker 2.22.0 from https://github.com/faker-ruby/faker.git (at master@5eb00cf)
#<Thread:0x00005651b3b2f628 threads.rb:19 run> terminated with exception (report_on_exception is true):
threads.rb:25:in `block (3 levels) in <main>': Thread 1 run occurrence 300029 expected 5 but got 0 (RuntimeError)
        from threads.rb:20:in `times'
        from threads.rb:20:in `each'
        from threads.rb:20:in `block (2 levels) in <main>'
Thread 0 completed successfully
threads.rb:25:in `block (3 levels) in <main>': Thread 1 run occurrence 300029 expected 5 but got 0 (RuntimeError)
        from threads.rb:20:in `times'
        from threads.rb:20:in `each'
        from threads.rb:20:in `block (2 levels) in <main>'

@thdaraujo thdaraujo reopened this Aug 13, 2022
@thdaraujo
Copy link
Contributor

just tested the solution proposed by @kiskoza and it seems to work.

[#2520 fixes #1700]

@thdaraujo
Copy link
Contributor

fixed by #2520

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ Issue: Has Attached PR 🪜Issue: With Reproduction Steps ↩ Needs Revisiting This issue or PR has been brought back and needs to be worked on.
Projects
None yet
Development

No branches or pull requests

5 participants