Rendering continues after Liquid::StackLevelError which can result in a lot of similar errors with recursion

[ADTC]

In Shopify, create a page template page.test2.liquid

{% layout none %}
{% assign aacounter = 1 %}
{% include 'aaaaa' %}

Create a snippet aaaaa.liquid

{{ aacounter }} {% assign aacounter = aacounter | plus: 1 %}
({% include 'aaaaa' %})
{% if aacounter < 10000 %}
[{% include 'aaaaa' %}]
{% endif %}
a 

Create a page and set it to the template. Launch the page.

Interestingly, because the snippet was included twice, it bypasses the MAX_DEPTH of 100 and continues the recursion forever. Eventually, Shopify server times out and kills the thread, then gives an error page. In the above example, 10000 would still work and give an output.

But if you set it to 15000 (or remove the check altogether), it will give the error page below:

This happens for both include and render. Related issue: #970

[dylanahsmith]

Interestingly, because the snippet was included twice, it bypasses the MAX_DEPTH of 100 and continues the recursion forever.

In the above example, 10000 would still work and give an output.

Saying it recurses forever isn't accurate given that as you say it does complete and given an output. It isn't that the MAX_DEPTH is bypassed, it is just looping a lot of times.

The outer-most include recurses the depth of 100, but since there are two includes, it branches out into a tree with a max depth of 100. That results in 2 ** 100 includes, which is a large enough iterations that it "effectively" would loop forever without a time limit. Similarly, (2 ** 100).times { } in ruby doesn't literally loop forever, but effectively it does.

Perhaps we should treat Liquid::StackLevelError similar to Liquid::MemoryError in that we shouldn't rescue and continue when we get that error so we fail fast.

[dylanahsmith]

Here is a script to easily show the problem locally

require 'liquid'

# test with a smaller max depth to make the behaviour clearer
Liquid::Block::MAX_DEPTH = 4

MemoryFileSystem = Struct.new(:templates) do
  def read_template_file(path)
    templates.fetch(path)
  end
end

source = <<~LIQUID
({% include 'recurse' %})
[{% include 'recurse' %}]
LIQUID

Liquid::Template.file_system = MemoryFileSystem.new('recurse' => source)

puts Liquid::Template.parse(source).render

which outputs

((((Liquid error: Nesting too deep)
[Liquid error: Nesting too deep]
)
[(Liquid error: Nesting too deep)
[Liquid error: Nesting too deep]
]
)
[((Liquid error: Nesting too deep)
[Liquid error: Nesting too deep]
)
[(Liquid error: Nesting too deep)
[Liquid error: Nesting too deep]
]
]
)
[(((Liquid error: Nesting too deep)
[Liquid error: Nesting too deep]
)
[(Liquid error: Nesting too deep)
[Liquid error: Nesting too deep]
]
)
[((Liquid error: Nesting too deep)
[Liquid error: Nesting too deep]
)
[(Liquid error: Nesting too deep)
[Liquid error: Nesting too deep]
]
]
]

but I think it should instead just output ((((Liquid error: Nesting too deep

[ADTC]

Saying it recurses forever isn't accurate given that as you say it does complete and given an output.

@dylanahsmith what I meant there is that it would recurse forever if I didn't have the limit of 10000. But that use of "forever" is theoretical. Eventually the server times out waiting for the output and just kills the process (and we get that grey page in response).

The only reason I put the limit of 10000 is so that I can force it to stop recursing at that point and give me the output so far. If I had no limit at all, ~it just won't stop (if it was given infinite time and resources).~ (see below)

[ADTC]

Or maybe what you're saying is that with two includes, the limit compounds to become 2 ** 100? What is that anyway? 2^100 = 1,267,650,600,228,229,401,496,703,205,376 ? Sorry I'm not familiar with Ruby.

Edit: Asked and answered: https://www.w3resource.com/ruby/ruby-arithmetic-operators.php So I'm right then... one nonillion two hundred and sixty-seven octillion six hundred fifty septillion six hundred sextillion and change, or "effectively" forever... or until the server gives up and kills the thread.

[ADTC]

Perhaps we should treat Liquid::StackLevelError similar to Liquid::MemoryError in that we shouldn't rescue and continue when we get that error so we fail fast.

I think it should instead just output ((((Liquid error: Nesting too deep

Hey, when we fail fast, do we send the partial output like that to the client, or do we discard the output and send an error page (like that gray page) instead? IMO it should be an error page with HTTP 508 Loop Detected* in this case. Partial outputs with HTTP 2xx can wreck the client and be difficult to detect and react to appropriately.

* The server detected an infinite loop while processing the request. (WebDAV; RFC 5842)