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

Drop support for ruby 2.7 and add 3.3 to test matrix #830

Merged
merged 9 commits into from
Feb 17, 2024

Conversation

pmor
Copy link
Member

@pmor pmor commented Dec 26, 2023

  • Bump minimum ruby version to 3.0

@swiknaba
Copy link
Contributor

A wild guess: https://github.com/countries/countries/blob/v5.7.1/spec/thread_safety_spec.rb#L14

100 Threads might not be enough to consistently get a thread safety issue considering Ruby's improved M:N Thread scheduler: https://github.com/ruby/ruby/blob/ruby_3_3/NEWS.md#mn-thread-scheduler

I tried to run it with 250 threads, but still don't get a race-condition. After 251 threads, my systems fails with Errno::EMFILE: Too many open files for the test that skips using a mutex.

I've injected the current thread ID into the cache object in each loop, which resulted in a unique ID for each thread, so the cache object is not thread-local I guess (to be expected?).

Other stuff I tried that didn't help:

  • directly call load_data! in each loop in each thread
  • adding sleep Kernel.rand(0.0001..0.001) in the synchronized block to have some artificial jitter with accessing the cache
  • adding random time delays in the file opening method
  • run for 250 threads and loop over 10k language codes

I ran tests on a M1 Max with 10 cores.

@pmor
Copy link
Member Author

pmor commented Feb 17, 2024

@swiknaba Thanks for digging into that. I haven't had any success either.

That code has been around for about 4 years, since before I was maintainer of the gem, and I don't think anyone reported thread safety issues since then. The #use_mutex? method is only there to force tests to run without the mutex anyway, so I think I'm just going to remove it and update the specs.

- Ensure Data.reset is called after locales are changed
- Ensure :en locale is set by default for all specs
- Do not freeze Country methods
@pmor pmor merged commit 7a958be into master Feb 17, 2024
13 checks passed
@pmor pmor deleted the ruby_versions_update branch February 17, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants