Merge pull request #771 from eregon/thread-safe-CachedEnvironment

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

.github/workflows/ci.yml

@@ -17,6 +17,8 @@ jobs:
         include:
           - { ruby: jruby, experimental: true }
           - { ruby: jruby-head, experimental: true }
+          - { ruby: truffleruby, experimental: true }
+          - { ruby: truffleruby-head, experimental: true }
           - { ruby: head, experimental: true }
 
     steps:

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 Get upgrade notes from Sprockets 3.x to 4.x at https://github.com/rails/sprockets/blob/master/UPGRADING.md
 
+- Fix thread safety of `Sprockets::CachedEnvironment` and `Sprockets::Cache::MemoryStore`. [#771](https://github.com/rails/sprockets/pull/771)
 - Add support for Rack 3.0. Headers set by sprockets will now be lower case. [#758](https://github.com/rails/sprockets/pull/758)
 - Make `Sprockets::Utils.module_include` thread safe on JRuby. [#759](https://github.com/rails/sprockets/pull/759)
 

lib/sprockets/cache/memory_store.rb

@@ -22,6 +22,7 @@ class MemoryStore
       def initialize(max_size = DEFAULT_MAX_SIZE)
         @max_size = max_size
         @cache = {}
+        @mutex = Mutex.new
       end
 
       # Public: Retrieve value from cache.
@@ -32,12 +33,14 @@ def initialize(max_size = DEFAULT_MAX_SIZE)
       #
       # Returns Object or nil or the value is not set.
       def get(key)
-        exists = true
-        value = @cache.delete(key) { exists = false }
-        if exists
-          @cache[key] = value
-        else
-          nil
+        @mutex.synchronize do
+          exists = true
+          value = @cache.delete(key) { exists = false }
+          if exists
+            @cache[key] = value
+          else
+            nil
+          end
         end
       end
 
@@ -50,24 +53,30 @@ def get(key)
       #
       # Returns Object value.
       def set(key, value)
-        @cache.delete(key)
-        @cache[key] = value
-        @cache.shift if @cache.size > @max_size
+        @mutex.synchronize do
+          @cache.delete(key)
+          @cache[key] = value
+          @cache.shift if @cache.size > @max_size
+        end
         value
       end
 
       # Public: Pretty inspect
       #
       # Returns String.
       def inspect
-        "#<#{self.class} size=#{@cache.size}/#{@max_size}>"
+        @mutex.synchronize do
+          "#<#{self.class} size=#{@cache.size}/#{@max_size}>"
+        end
       end
 
       # Public: Clear the cache
       #
       # Returns true
       def clear(options=nil)
-        @cache.clear
+        @mutex.synchronize do
+          @cache.clear
+        end
         true
       end
     end

lib/sprockets/cached_environment.rb

@@ -16,11 +16,11 @@ def initialize(environment)
       initialize_configuration(environment)
 
       @cache   = environment.cache
-      @stats   = {}
-      @entries = {}
-      @uris    = {}
-      @processor_cache_keys = {}
-      @resolved_dependencies = {}
+      @stats   = Concurrent::Map.new
+      @entries = Concurrent::Map.new
+      @uris    = Concurrent::Map.new
+      @processor_cache_keys = Concurrent::Map.new
+      @resolved_dependencies = Concurrent::Map.new
     end
 
     # No-op return self as cached environment.
@@ -31,27 +31,27 @@ def cached
 
     # Internal: Cache Environment#entries
     def entries(path)
-      @entries.fetch(path){ @entries[path] = super(path) }
+      @entries.fetch_or_store(path) { super(path) }
     end
 
     # Internal: Cache Environment#stat
     def stat(path)
-      @stats.fetch(path){ @stats[path] = super(path) }
+      @stats.fetch_or_store(path) { super(path) }
     end
 
     # Internal: Cache Environment#load
     def load(uri)
-      @uris.fetch(uri){ @uris[uri] = super(uri) }
+      @uris.fetch_or_store(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) }
+      @processor_cache_keys.fetch_or_store(str) { super(str) }
     end
 
     # Internal: Cache Environment#resolve_dependency
     def resolve_dependency(str)
-      @resolved_dependencies.fetch(str){ @resolved_dependencies[str] = super(str) }
+      @resolved_dependencies.fetch_or_store(str) { super(str) }
     end
 
     private

lib/sprockets/uri_utils.rb

@@ -53,7 +53,7 @@ def split_file_uri(uri)
         path = path[1..-1]
       end
 
-      [scheme, host, path, query]
+      [scheme, host || '', path, query]
     end
 
     # Internal: Join file: URI component parts into String.

test/test_encoding_utils.rb

@@ -52,6 +52,8 @@ def test_fail_to_unmarshal_older_major_version
   end
 
   def test_fail_to_unmarshal_not_enough_data
+    skip "TypeError on TruffleRuby" if RUBY_ENGINE == "truffleruby"
+
     assert_raises ArgumentError do
       Marshal.load("")
     end

test/test_erb_processor.rb

@@ -32,6 +32,8 @@ def test_compile_erb_template_that_depends_on_env
     old_env_value = ::ENV['ERB_ENV_TEST_VALUE']
     ::ENV['ERB_ENV_TEST_VALUE'] = 'success'
 
+    skip 'https://github.com/oracle/truffleruby/issues/2810' if RUBY_ENGINE == 'truffleruby'
+
     root = File.expand_path("../fixtures", __FILE__)
     environment = Sprockets::Environment.new(root)
     environment.append_path 'default'
@@ -56,6 +58,8 @@ def test_compile_erb_template_that_depends_on_env
   def test_compile_erb_template_that_depends_on_empty_env
     old_env_value = ::ENV.delete('ERB_ENV_TEST_VALUE')
 
+    skip 'https://github.com/oracle/truffleruby/issues/2810' if RUBY_ENGINE == 'truffleruby'
+
     root = File.expand_path("../fixtures", __FILE__)
     environment = Sprockets::Environment.new(root)
     environment.append_path 'default'

test/test_uri_utils.rb

@@ -39,23 +39,23 @@ def test_split_file_uri
     assert_equal ['file', 'localhost', '/etc/fstab', nil], parts
 
     parts = split_file_uri("file:///etc/fstab")
-    assert_equal ['file', nil, '/etc/fstab', nil], parts
+    assert_equal ['file', '', '/etc/fstab', nil], parts
 
     parts = split_file_uri("file:///usr/local/bin/ruby%20on%20rails")
-    assert_equal ['file', nil, '/usr/local/bin/ruby on rails', nil], parts
+    assert_equal ['file', '', '/usr/local/bin/ruby on rails', nil], parts
 
     parts = split_file_uri("file:///usr/local/var/github/app/assets/javascripts/application.js")
-    assert_equal ['file', nil, '/usr/local/var/github/app/assets/javascripts/application.js', nil], parts
+    assert_equal ['file', '', '/usr/local/var/github/app/assets/javascripts/application.js', nil], parts
 
     if DOSISH
       parts = split_file_uri("file:///C:/Documents%20and%20Settings/davris/FileSchemeURIs.doc")
-      assert_equal ['file', nil, 'C:/Documents and Settings/davris/FileSchemeURIs.doc', nil], parts
+      assert_equal ['file', '', 'C:/Documents and Settings/davris/FileSchemeURIs.doc', nil], parts
 
       parts = split_file_uri("file:///D:/Program%20Files/Viewer/startup.htm")
-      assert_equal ['file', nil, 'D:/Program Files/Viewer/startup.htm', nil], parts
+      assert_equal ['file', '', 'D:/Program Files/Viewer/startup.htm', nil], parts
 
       parts = split_file_uri("file:///C:/Program%20Files/Music/Web%20Sys/main.html?REQUEST=RADIO")
-      assert_equal ['file', nil, 'C:/Program Files/Music/Web Sys/main.html', 'REQUEST=RADIO'], parts
+      assert_equal ['file', '', 'C:/Program Files/Music/Web Sys/main.html', 'REQUEST=RADIO'], parts
     end
   end