Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

AV::Digestor thread safety fixes #11664

Merged
merged 1 commit into from

2 participants

@thedarkone

This fixes potential thread safety issues introduced in 09f6fe1. The problem with the original code was that the "recursion-stopping" nil values could be seen by other threads. See 09f6fe1's comments for more info.

This is targeting master, but a similar commit was also pushed into 4-0-stable: 247aa53.

ping @rafaelfranca.

@thedarkone thedarkone AV::Digestor thread safety fixes.
This fixes potential thread safety issues introduced in 09f6fe1. The problem
with the original code was that the "recursion-stopping" `nil` values could
be seen by other threads.
650810f
@rafaelfranca rafaelfranca merged commit 77cf5cf into rails:master

1 check failed

Details default The Travis CI build is in progress
@rafaelfranca

I'll backport to 4-0-stable after the build is finished

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 30, 2013
  1. @thedarkone

    AV::Digestor thread safety fixes.

    thedarkone authored
    This fixes potential thread safety issues introduced in 09f6fe1. The problem
    with the original code was that the "recursion-stopping" `nil` values could
    be seen by other threads.
This page is out of date. Refresh to see the latest.
Showing with 29 additions and 9 deletions.
  1. +29 −9 actionview/lib/action_view/digestor.rb
View
38 actionview/lib/action_view/digestor.rb
@@ -1,20 +1,40 @@
require 'thread_safe'
require 'action_view/dependency_tracker'
+require 'monitor'
module ActionView
class Digestor
cattr_reader(:cache)
- @@cache = ThreadSafe::Cache.new
-
- def self.digest(name, format, finder, options = {})
- cache_key = ([name, format] + Array.wrap(options[:dependencies])).join('.')
- @@cache.fetch(cache_key) do
- @@cache[cache_key] ||= nil if options[:partial] # Prevent re-entry
+ @@cache = ThreadSafe::Cache.new
+ @@digest_monitor = Monitor.new
+
+ class << self
+ def digest(name, format, finder, options = {})
+ cache_key = ([name, format] + Array.wrap(options[:dependencies])).join('.')
+ # this is a correctly done double-checked locking idiom
+ # (ThreadSafe::Cache's lookups have volatile semantics)
+ @@cache[cache_key] || @@digest_monitor.synchronize do
+ @@cache.fetch(cache_key) do # re-check under lock
+ compute_and_store_digest(cache_key, name, format, finder, options)
+ end
+ end
+ end
- klass = options[:partial] || name.include?("/_") ? PartialDigestor : Digestor
- digest = klass.new(name, format, finder, options).digest
+ private
+ def compute_and_store_digest(cache_key, name, format, finder, options) # called under @@digest_monitor lock
+ klass = if options[:partial] || name.include?("/_")
+ # Prevent re-entry or else recursive templates will blow the stack.
+ # There is no need to worry about other threads seeing the +false+ value,
+ # as they will then have to wait for this thread to let go of the @@digest_monitor lock.
+ pre_stored = @@cache.put_if_absent(cache_key, false).nil? # put_if_absent returns nil on insertion
+ PartialDigestor
+ else
+ Digestor
+ end
- @@cache[cache_key] = digest # Store the value
+ @@cache[cache_key] = digest = klass.new(name, format, finder, options).digest # Store the actual digest
+ ensure
+ @@cache.delete_pair(cache_key, false) if pre_stored && !digest # something went wrong, make sure not to corrupt the @@cache
end
end
Something went wrong with that request. Please try again.