Skip to content

Commit

Permalink
Fix "Stack Level Too Deep" error when rendering recursive partials
Browse files Browse the repository at this point in the history
When rendering recursive partial Action View is trying to generate the
view digest infinitly causing a stack level error.

Fixes #11340
  • Loading branch information
rafaelfranca committed Jul 7, 2013
1 parent a67cc28 commit 09f6fe1
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 3 deletions.
6 changes: 6 additions & 0 deletions actionview/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
* Fix "Stack Level Too Deep" error when redering recursive partials.

Fixes #11340.

*Rafael Mendonça França*

* Added an `enforce_utf8` hash option for `form_tag` method.

Control to output a hidden input tag with name `utf8` without monkey
Expand Down
10 changes: 7 additions & 3 deletions actionview/lib/action_view/digestor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,14 @@ class Digestor
@@cache = ThreadSafe::Cache.new

def self.digest(name, format, finder, options = {})
cache_key = [name, format] + Array.wrap(options[:dependencies])
@@cache[cache_key.join('.')] ||= begin
cache_key = ([name, format] + Array.wrap(options[:dependencies])).join('.')
@@cache.fetch(cache_key) do
@@cache[cache_key] ||= nil if options[:partial] # Prevent re-entry

klass = options[:partial] || name.include?("/_") ? PartialDigestor : Digestor
klass.new(name, format, finder, options).digest
digest = klass.new(name, format, finder, options).digest

@@cache[cache_key] = digest # Store the value
end
end

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<%= render 'recursion' %>
1 change: 1 addition & 0 deletions actionview/test/fixtures/digestor/level/recursion.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<%= render 'recursion' %>
25 changes: 25 additions & 0 deletions actionview/test/template/digestor_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,31 @@ def test_nested_template_directory
end
end

def test_recursion_in_renders
assert digest("level/recursion") # assert recursion is possible
assert_not_nil digest("level/recursion") # assert digest is stored
end

def test_chaning_the_top_templete_on_recursion
assert digest("level/recursion") # assert recursion is possible

assert_digest_difference("level/recursion") do
change_template("level/recursion")
end

assert_not_nil digest("level/recursion") # assert digest is stored
end

def test_chaning_the_partial_templete_on_recursion
assert digest("level/recursion") # assert recursion is possible

assert_digest_difference("level/recursion") do
change_template("level/_recursion")
end

assert_not_nil digest("level/recursion") # assert digest is stored
end

def test_dont_generate_a_digest_for_missing_templates
assert_equal '', digest("nothing/there")
end
Expand Down

8 comments on commit 09f6fe1

@rafaelfranca
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thedarkone any thread safety concerns here?

@thedarkone
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca there's a problem with other threads seeing the "pre-stored" nil value, the simplest solution is to have mutex around digest computation (this way if a different thread sees a recursion blocking nil it defaults to grabbing a mutex to compute the value, this way it is forced to wait for the current thread to successfully compute and replace the placeholder nil). I also think the code needs to be defensive and have an ensure block that cleans up the @@cache if the klass.new.digest call blows up.

require 'monitor'
@@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_digest(cache_key, name, format, finder, options)
      end
    end
  end

  private
  def compute_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 = 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

@rafaelfranca
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍. Could you open a PR?

@wyaeld
Copy link
Contributor

@wyaeld wyaeld commented on 09f6fe1 Oct 14, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca @thedarkone, I think we've found a bug with this change. Background: Moved off 4.0.0 gem today to 4-0-stable because we have a recursive template being used, ran smack into this bug.

We have a navigation menu that is a 3-level deep tree structure, and uses a recursive template.
When using the cache, via <% cache template_section do %> ... in the partial, the following happens

First-read on server restart: digest-computed, first fetch uses digest in key, all others use false

D, [2013-10-14T17:28:26.167201 #28567] DEBUG -- :   TemplateSection Load (1.5ms)  SELECT "template_sections".* FROM "template_sections" WHERE "template_sections"."template_id" = $1  [["template_id", 1]]
D, [2013-10-14T17:28:26.167281 #28567] DEBUG -- :   TemplateSection Load (1.5ms)  SELECT "template_sections".* FROM "template_sections" WHERE "template_sections"."template_id" = $1  [["template_id", 1]]
I, [2013-10-14T17:28:26.175609 #28567]  INFO -- : Cache digest for template_sections/_template_section.html: 2522b8090b7a9ece60ab352e9abd5740
I, [2013-10-14T17:28:26.175666 #28567]  INFO -- : Cache digest for template_sections/_template_section.html: 2522b8090b7a9ece60ab352e9abd5740
I, [2013-10-14T17:28:26.176220 #28567]  INFO -- : Read fragment views/template_sections/1-20131013203234122198000/2522b8090b7a9ece60ab352e9abd5740 (0.2ms)
I, [2013-10-14T17:28:26.176244 #28567]  INFO -- : Read fragment views/template_sections/1-20131013203234122198000/2522b8090b7a9ece60ab352e9abd5740 (0.2ms)
I, [2013-10-14T17:28:26.176524 #28567]  INFO -- : Read fragment views/template_sections/2-20131013203234155183000/false (0.1ms)
I, [2013-10-14T17:28:26.176546 #28567]  INFO -- : Read fragment views/template_sections/2-20131013203234155183000/false (0.1ms)
I, [2013-10-14T17:28:26.176777 #28567]  INFO -- : Read fragment views/template_sections/3-20131013203234164048000/false (0.1ms)
I, [2013-10-14T17:28:26.176798 #28567]  INFO -- : Read fragment views/template_sections/3-20131013203234164048000/false (0.1ms)
I, [2013-10-14T17:28:26.176983 #28567]  INFO -- : Read fragment views/template_sections/4-20131013203234171049000/false (0.0ms)
I, [2013-10-14T17:28:26.177015 #28567]  INFO -- : Read fragment views/template_sections/4-20131013203234171049000/false (0.0ms)
I, [2013-10-14T17:28:26.177346 #28567]  INFO -- : Read fragment views/template_sections/5-20131013203234178758000/false (0.1ms)


Second & subsequent reads: all fetches use false value in place of digest.

D, [2013-10-14T17:18:09.935522 #28377] DEBUG -- :   TemplateSection Load (0.7ms)  SELECT "template_sections".* FROM "template_sections" WHERE "template_sections"."template_id" = $1 AND "template_sections"."parent_id" IS NULL ORDER BY "lft"  [["template_id", 1]]
D, [2013-10-14T17:18:09.935583 #28377] DEBUG -- :   TemplateSection Load (0.7ms)  SELECT "template_sections".* FROM "template_sections" WHERE "template_sections"."template_id" = $1 AND "template_sections"."parent_id" IS NULL ORDER BY "lft"  [["template_id", 1]]
I, [2013-10-14T17:18:09.937908 #28377]  INFO -- : Read fragment views/template_sections/1-20131013203234122198000/false (0.2ms)
I, [2013-10-14T17:18:09.937947 #28377]  INFO -- : Read fragment views/template_sections/1-20131013203234122198000/false (0.2ms)
I, [2013-10-14T17:18:09.938274 #28377]  INFO -- : Read fragment views/template_sections/4-20131013203234171049000/false (0.1ms)
I, [2013-10-14T17:18:09.938304 #28377]  INFO -- : Read fragment views/template_sections/4-20131013203234171049000/false (0.1ms)
I, [2013-10-14T17:18:09.938587 #28377]  INFO -- : Read fragment views/template_sections/16-20131013203234279342000/false (0.1ms)
I, [2013-10-14T17:18:09.938620 #28377]  INFO -- : Read fragment views/template_sections/16-20131013203234279342000/false (0.1ms)
I, [2013-10-14T17:18:09.938941 #28377]  INFO -- : Read fragment views/template_sections/19-20131013203234301609000/false (0.1ms)
I, [2013-10-14T17:18:09.938972 #28377]  INFO -- : Read fragment views/template_sections/19-20131013203234301609000/false (0.1ms)
I, [2013-10-14T17:18:09.939247 #28377]  INFO -- : Read fragment views/template_sections/24-20131013203234342208000/false (0.1ms)
I, [2013-10-14T17:18:09.939276 #28377]  INFO -- : Read fragment views/template_sections/24-20131013203234342208000/false (0.1ms)
I, [2013-10-14T17:18:09.939559 #28377]  INFO -- : Read fragment views/template_sections/31-20131013203234397174000/false (0.1ms)

Under simple testing, and the tests included, the code appears to work because the cache_key with false is still a valid lookup, however it causes breaking problems.

  1. Any updates to the template won't be reflected, because now all the reads are working off a key that doesn't use the current digest value.
  2. Objects that have multiple partial representations won't work, because its dropping the digest part of the key, and therefore you just get whichever partial had been thrown in against the "false" key first.

I'll love to offer the solution, but this code it a bit too complex for me to be comfortable I understand the dimensions well enough, but I'm happy to help test any proposed solutions.

@rafaelfranca
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you file an issue so we can track it? Thank you for reporting.

@wyaeld
Copy link
Contributor

@wyaeld wyaeld commented on 09f6fe1 Oct 14, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done #12521

@rafaelfranca
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@wyaeld
Copy link
Contributor

@wyaeld wyaeld commented on 09f6fe1 Oct 14, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it helps, this is on 2.0.0@p247, running unicorn.

Please sign in to comment.