Infinite recursion with recursing xi:include with TemplateLoader

[jaraco@…]

I've discovered an issue where a self-referencing XML template will trigger an infinite recursion when the template is rendered against a very shallow dataset. I've distilled the problem down to a very small replication of the failure. In distilling, I found that I could only replicate the failure when using the TemplateLoader. If I instead use a hand-crafted SimpleLoader, the terminated recursion does not occur (and the document renders properly).

For easy viewing, the script ​can be viewed here, and I will attach it as well. Note that the script as written does not fail unless the final line is uncommented.

Because the template can be rendered successfully against the dataset with a simple loader, this behavior strikes me as a deficiency of the TemplateLoader or of the xi:include handler when used in conjunction with the TemplateLoader.

This issue is blocking our ability to render a data structure which is inherently recursive, so if you can recommend a workaround, I'm eager to try anything.

[jaraco@…]

This bug seems very similar to one I posted a couple of years ago - #409. Unfortunately, I don't see why that issue was closed.

[Jason R. Coombs <jaraco@…>]

It appears the problem occurs in template/base.py:499 where the stream is inlined. If auto_reload is set on the loader and False, the include is inlined. However, inlining a self-reference leads to an infinite recursion. Setting auto_reload=True on the loader works around the issue.

[hodgestar]

Ideally the inlining should protect against:

• recursion (I think here _prepare could just maintain a set of template hrefs inlined so far.
• very deep inlining (I guess we could use the set of hrefs here again and if the set grows very large, stop inlining).

Thanks for the bug report and digging, jaraco!

[hodgestar]

Patch that checks that inlined includes don't recurse.

[hodgestar]

Demonstration of how auto_reload=False triggers the bug.

[hodgestar]

I attached a patch. I'd appreciate some feedback, specifically on:

• does this solve your original issue?
• the patch changes the method signature of _prepare and some of the details around how template preparation works. Since these are all _ prefixed methods, I'm inclined to say this patch can into 0.7.x. Do others agree?

I did implement a check for very deep inlining but then decided it wasn't worth it -- if someone has a very deep static template hierarchy, I don't want to change the behaviour for them.

I also wondered whether inlining really buys us anything -- typically template loading and preparation is cached anyway. Probably it does because we still flatten the stream a lot, but it's not entirely clear to me. Investigating properly is something for another time though.

[Jason R. Coombs <jaraco@…>]

Hodgestar, thanks for the rapid response. Unfortunately, while the patch does appear to correct the recursion I identified, it doesn't prevent the 'Recursion detected' error I get when I run our test suite with this updated code. I do suspect, however, there may be issues with the test framework itself (py.test). I'll do more investigation and report back.

[hodgestar]

I wonder if py.test is attempting to solve the halting problem and failing? :)

[Jason R. Coombs <jaraco@…>]

I've found something slightly surprising. Even bypassing the test suite and invoking the test directly, I encounter a RuntimeError for recursion, so it seems the patch doesn't prevent the error in this particular use case. I will have to investigate more deeply to better understand how the minimal example differs from the real-world failure.

[Jason R. Coombs <jaraco@…>]

a more aggressive example with two templates

[Jason R. Coombs <jaraco@…>]

I've added a new script which replicates the issue even with the patch applied. This example more closely resembles the real-world implementation we have where a top-level template includes a template which then includes itself.

Thanks again for the help on this issue. I respect that's it's a challenging problem and I'm very pleased with the progress.

[Jason R. Coombs <jaraco@…>]

Replying to hodgestar:

I also wondered whether inlining really buys us anything -- typically template loading and preparation is cached anyway. Probably it does because we still flatten the stream a lot, but it's not entirely clear to me. Investigating properly is something for another time though.

Perhaps inlining should be an opt-in feature given the inherent challenges of inlining objects which allowed to be self-referential. Or alternatively, perhaps inlining could be disabled if it encounters a RuntimeError? indicating too much recursion.

[hodgestar]

Checked inlining patch v2.

[hodgestar]

Oops. That was due to a small bug in the original patch (it added tmpl.filename to the set of inlined templates instead of tmpl.filepath). Fixed patch attached.

[hodgestar]

Patch committed to trunk in r1257 and merged into 0.7.x (r1258) and 0.6.x (r1259).

Thanks for the bug report!

[Jason R. Coombs <jaraco@…>]

Thanks for the rapid response!