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.

Sign up for GitHub

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

Jump to bottom

Sprockets::CachedEnvironment is not thread-safe #772

Closed
eregon opened this issue Dec 19, 2022 · 7 comments · Fixed by #771
Closed

Sprockets::CachedEnvironment is not thread-safe #772

eregon opened this issue Dec 19, 2022 · 7 comments · Fixed by #771

Comments

@eregon
Copy link
Contributor

eregon commented Dec 19, 2022 •
edited
Loading

Sprockets::CachedEnvironment is not thread-safe:

sprockets/lib/sprockets/cached_environment.rb

Lines 34 to 54 in 1276b43

@entries.fetch(path){ @entries[path] = super(path) }
end
# Internal: Cache Environment#stat
def stat(path)
@stats.fetch(path){ @stats[path] = super(path) }
end
# Internal: Cache Environment#load
def load(uri)
@uris.fetch(uri){ @uris[uri] = super(uri) }
end
# Internal: Cache Environment#processor_cache_key
def processor_cache_key(str)
@processor_cache_keys.fetch(str){ @processor_cache_keys[str] = super(str) }
end
# Internal: Cache Environment#resolve_dependency
def resolve_dependency(str)
@resolved_dependencies.fetch(str){ @resolved_dependencies[str] = super(str) }

Specifically it mutates Hash instances concurrently.

Expected behavior

It should work.

Actual behavior

It crashes with various thread safety-related issues.
For instance oracle/truffleruby#2808 and oracle/truffleruby#2804.

I tried the obvious approach of using Concurrent::Map.new + compute_if_absent instead of a Hash in #771, but that doesn't work because Sprockets calls Sprockets::CachedEnvironment#load recursively (and on the same CachedEnvironment instance), e.g. (on CRuby):

$ bundle exec bin/sprockets -I test/fixtures/asset $PWD/test/fixtures/asset/application.js
/home/eregon/.bundle/ruby/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/collection/map/mri_map_backend.rb:25:in `synchronize': deadlock; recursive locking (ThreadError)
	from /home/eregon/.bundle/ruby/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/collection/map/mri_map_backend.rb:25:in `compute_if_absent'
	from /home/eregon/code/sprockets/lib/sprockets/cached_environment.rb:45:in `load'
	from /home/eregon/code/sprockets/lib/sprockets/bundle.rb:27:in `call'
	from /home/eregon/code/sprockets/lib/sprockets/processor_utils.rb:84:in `call_processor'
	from /home/eregon/code/sprockets/lib/sprockets/processor_utils.rb:66:in `block in call_processors'
	from /home/eregon/code/sprockets/lib/sprockets/processor_utils.rb:65:in `reverse_each'
	from /home/eregon/code/sprockets/lib/sprockets/processor_utils.rb:65:in `call_processors'
	from /home/eregon/code/sprockets/lib/sprockets/loader.rb:182:in `load_from_unloaded'
	from /home/eregon/code/sprockets/lib/sprockets/loader.rb:59:in `block in load'
	from /home/eregon/code/sprockets/lib/sprockets/loader.rb:337:in `fetch_asset_from_dependency_cache'
	from /home/eregon/code/sprockets/lib/sprockets/loader.rb:43:in `load'
	from /home/eregon/code/sprockets/lib/sprockets/cached_environment.rb:45:in `block in load'
	from /home/eregon/.bundle/ruby/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/collection/map/non_concurrent_map_backend.rb:31:in `compute_if_absent'
	from /home/eregon/.bundle/ruby/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/collection/map/mri_map_backend.rb:25:in `block in compute_if_absent'
	from /home/eregon/.bundle/ruby/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/collection/map/mri_map_backend.rb:25:in `synchronize'
	from /home/eregon/.bundle/ruby/3.1.0/gems/concurrent-ruby-1.1.10/lib/concurrent-ruby/concurrent/collection/map/mri_map_backend.rb:25:in `compute_if_absent'
	from /home/eregon/code/sprockets/lib/sprockets/cached_environment.rb:45:in `load'
	from /home/eregon/code/sprockets/lib/sprockets/base.rb:81:in `find_asset'
	from /home/eregon/code/sprockets/lib/sprockets/environment.rb:32:in `find_asset'
	from bin/sprockets:90:in `<top (required)>'
	from /home/eregon/.rubies/ruby-3.1.2/lib/ruby/gems/3.1.0/gems/bundler-2.3.26/lib/bundler/cli/exec.rb:58:in `load'
	from /home/eregon/.rubies/ruby-3.1.2/lib/ruby/gems/3.1.0/gems/bundler-2.3.26/lib/bundler/cli/exec.rb:58:in `kernel_load'
	from /home/eregon/.rubies/ruby-3.1.2/lib/ruby/gems/3.1.0/gems/bundler-2.3.26/lib/bundler/cli/exec.rb:23:in `run'
	from /home/eregon/.rubies/ruby-3.1.2/lib/ruby/gems/3.1.0/gems/bundler-2.3.26/lib/bundler/cli.rb:486:in `exec'
	from /home/eregon/.rubies/ruby-3.1.2/lib/ruby/gems/3.1.0/gems/bundler-2.3.26/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
	from /home/eregon/.rubies/ruby-3.1.2/lib/ruby/gems/3.1.0/gems/bundler-2.3.26/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
	from /home/eregon/.rubies/ruby-3.1.2/lib/ruby/gems/3.1.0/gems/bundler-2.3.26/lib/bundler/vendor/thor/lib/thor.rb:392:in `dispatch'
	from /home/eregon/.rubies/ruby-3.1.2/lib/ruby/gems/3.1.0/gems/bundler-2.3.26/lib/bundler/cli.rb:31:in `dispatch'
	from /home/eregon/.rubies/ruby-3.1.2/lib/ruby/gems/3.1.0/gems/bundler-2.3.26/lib/bundler/vendor/thor/lib/thor/base.rb:485:in `start'
	from /home/eregon/.rubies/ruby-3.1.2/lib/ruby/gems/3.1.0/gems/bundler-2.3.26/lib/bundler/cli.rb:25:in `start'
	from /home/eregon/.rubies/ruby-3.1.2/lib/ruby/gems/3.1.0/gems/bundler-2.3.26/exe/bundle:48:in `block in <top (required)>'
	from /home/eregon/.rubies/ruby-3.1.2/lib/ruby/gems/3.1.0/gems/bundler-2.3.26/lib/bundler/friendly_errors.rb:120:in `with_friendly_errors'
	from /home/eregon/.rubies/ruby-3.1.2/lib/ruby/gems/3.1.0/gems/bundler-2.3.26/exe/bundle:36:in `<top (required)>'
	from /home/eregon/.rubies/ruby-3.1.2/bin/bundle:25:in `load'
	from /home/eregon/.rubies/ruby-3.1.2/bin/bundle:25:in `<main>'

System configuration

  • Sprockets version - master
  • Ruby version - 3.1

Example App (Reproduction) - THIS IS IMPORTANT YOUR ISSUE LIKELY WILL NOT BE RESOLVED WITHOUT THIS

Running the tests on TruffleRuby (bundle exec rake) on TruffleRuby reproduce this issue.
Also the command-line above is an easy way to reproduce it on branch #771.
This was referenced Dec 19, 2022
Merged
Closed
Closed
@eregon
Copy link
Contributor Author

eregon commented Dec 19, 2022

What I need help with is whether this recursive call is intended and if we can avoid it.
For instance, does Sprockets::Bundle.call needs to use the cached environment or could it call Sprockets::Loader#load to avoid the recursion?

@eregon
Copy link
Contributor Author

eregon commented Dec 19, 2022

I've quickly tried to replace env.load calls in Sprockets::Bundle.call, but it also happens in SourceMapProcessor.call.
Maybe we should detect the recursion in CachedEnvironment if Sprockets relies on these recursive calls causing cache miss to work fine?

@eregon
Copy link
Contributor Author

eregon commented Dec 19, 2022

@byroot Could you help me with this? Maybe we can have a call or something to discuss the solutions.

@eregon
Copy link
Contributor Author

eregon commented Dec 19, 2022

Note: we also need to care about the value being nil possibly for those caches since #723.

@eregon
Copy link
Contributor Author

eregon commented Dec 19, 2022

Sprockets::Cache::MemoryStore (https://github.com/rails/sprockets/blob/main/lib/sprockets/cache/memory_store.rb) has similar thread safety problems, and that matches the other backtraces in the TruffleRuby issues linked in the description.

@eregon
Copy link
Contributor Author

eregon commented Dec 19, 2022 •
edited
Loading

I have found a solution using Concurrent::Map#fetch_or_store and updated #771.
And just a Mutex for Sprockets::Cache::MemoryStore.
Sorry for all the messages/notifications here, I'm unfamiliar with Sprockets so wanted to make notes and talking points.

I'm still interested to understand a bit better about Sprockets and whether it's OK to cache miss 2+ times for the same key in CachedEnvironment (was already the case).

eregon added a commit to eregon/sprockets that referenced this issue Dec 19, 2022
@eregon
Add TruffleRuby in CI
58eb15d 
* 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
@casperisfine
Copy link

whether it's OK to cache miss 2+ times for the same key in CachedEnvironment (was already the case).

Yeah I think it's fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix thread safety of Sprockets::CachedEnvironment and Sprockets::Cache::MemoryStore eregon/sprockets
2 participants
@eregon @casperisfine