search

rails / sprockets

Rack-based asset packaging system
MIT License
932 stars 792 forks source link

Sprockets::CachedEnvironment is not thread-safe #772

Closed eregon closed 1 year ago

eregon commented 1 year ago

Sprockets::CachedEnvironment is not thread-safe: https://github.com/rails/sprockets/blob/1276b431e2e4c1099dae1b3ff76adc868c863ddd/lib/sprockets/cached_environment.rb#L34-L54 Specifically it mutates Hash instances concurrently.

Expected behavior

It should work.

Actual behavior

It crashes with various thread safety-related issues. For instance https://github.com/oracle/truffleruby/issues/2808 and https://github.com/oracle/truffleruby/issues/2804.

I tried the obvious approach of using Concurrent::Map.new + compute_if_absent instead of a Hash in https://github.com/rails/sprockets/pull/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

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 https://github.com/rails/sprockets/pull/771.

eregon commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

eregon commented 1 year ago

Note: we also need to care about the value being nil possibly for those caches since https://github.com/rails/sprockets/pull/723.

eregon commented 1 year ago

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 commented 1 year ago

I have found a solution using Concurrent::Map#fetch_or_store and updated https://github.com/rails/sprockets/pull/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).

casperisfine commented 1 year ago

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.