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

Fix thread safety of Sprockets::CachedEnvironment and Sprockets::Cache::MemoryStore #771

Merged
merged 6 commits into from
Dec 20, 2022

Conversation

eregon
Copy link
Contributor

@eregon eregon commented Dec 19, 2022

Fixes #772

* Unfortunately this on its own does not work yet due to Sprockets
  recursively calling Sprockets::CachedEnvironment#load.
* Reproduce the recursion issue with:
  bundle exec bin/sprockets -I test/fixtures/asset $PWD/test/fixtures/asset/application.js
@eregon eregon force-pushed the thread-safe-CachedEnvironment branch from 0ec1993 to b3c69c0 Compare December 19, 2022 18:02
@eregon eregon marked this pull request as ready for review December 19, 2022 18:03
* This does not hold any lock while computing the missing key,
  so avoids the problem of having a lock while recursing when
  using #compute_if_absent.
* As the docs say:
  https://ruby-concurrency.github.io/concurrent-ruby/master/Concurrent/Map.html#fetch_or_store-instance_method
  "The store can overwrite other concurrently stored value.".
  Which was already possibly before, Hash#fetch does not have the
  guarantee to execute the block only once per key, even on CRuby.
@eregon eregon force-pushed the thread-safe-CachedEnvironment branch from b3c69c0 to 9d461f1 Compare December 19, 2022 19:43
@eregon eregon changed the title Make Sprockets::CachedEnvironment thread-safe by using Concurrent::Map Fix thread safety of Sprockets::CachedEnvironment and Sprockets::Cache::MemoryStore Dec 19, 2022
* This should help catching concurrency issues much faster.
* Notably, the two issues of rails#772 reproduce by running the test suite on TruffleRuby.
* Skip tests which do not pass yet on TrufffleRuby.
  oracle/truffleruby#2810
* host is "" instead of nil, following the upstream change:
  ruby/uri@81263c9
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.

Sprockets::CachedEnvironment is not thread-safe
3 participants