GH-1621 - fix recursion error in TT after longjump

[abeverley]

Fix problem described in #1621. Rebuild the Template::Toolkit engine if it doesn't complete its render, otherwise a recursion error is thrown for any subsequent template processes. This is prevalent when using Plugin::LogReport which makes a call to redirect if a fatal exception occurs (which could happen during the render of a template).

There is one outstanding problem with this PR, which is that there is a very odd error on completion of the test, normally:

Can't call method "undefined" on an undefined value but can sometimes be something random like Can't locate object method "undefined" via package "Correct content" (perhaps you forgot to load "Correct content"?). or Can't locate object method "undefined" via package "Plack::Util::Prototype::set" (perhaps you forgot to load "Plack::Util::Prototype::set"?)

This will need to be fixed before any merge. I have spent hours trying to track it down (including via the Perl debugger) but it is a mystery. Does anyone else have any idea where it is coming from? It does not happen when either using another templating engine or removing the redirect call during the render.

[1nickt]

I have also encountered the following errors with the test: Can't locate object method "undefined" via package "Test::More::done_testing" (perhaps you forgot to load "Test::More::done_testing"?). Can't locate object method "undefined" via package "140208495660944" (perhaps you forgot to load "140208495660944"?).

[1nickt]

Also I note that Test2 is in the mix somehow...

Test2::API::CODE(0x7fed0a1b84c0)(/Users/ntonkin/.perlbrew/libs/perl-5.32.1@meta/lib/perl5/Test2/API.pm:71):
71:     INIT { eval 'END { test2_set_is_end() }; 1' or die $@ }

(however testing shows that the error is not coming from test2_set_is_end()...)

[veryrusty]

This feels like evil layered on evil (my fault for introducing with_return under the hood many moons ago!). The bug also potentially impacts any templating engine, not just Template::Toolkit. A solution that works everywhere would be great.

I'd like to refine the following further, but its late here and I'll have to come back to in tomorrow. It's also a little evil, but may work for all template engines (and passes the tests in this PR); Change the with_return sub before rendering, catching the first response. Should be more efficient than creating a new Template engine object. Would be nice to halt the render (if possible) after local_response is first set .. but the following may be sufficient.

veryrusty:d2(abeverley-gh-1621) russellj$ git diff
diff --git a/lib/Dancer2/Core/App.pm b/lib/Dancer2/Core/App.pm
index 6c9ea673..cdc556b8 100644
--- a/lib/Dancer2/Core/App.pm
+++ b/lib/Dancer2/Core/App.pm
@@ -884,6 +884,19 @@ sub template {
         and $self->setup_session;

     # return content
+    if ($self->has_with_return) {
+        my $old_with_return = $self->with_return;
+        my $local_response;
+        $self->set_with_return( sub {
+            $local_response ||= shift;
+        });
+        my $content = $template->process( @_ );
+        $self->set_with_return($old_with_return);
+        if ($local_response) {
+            $self->with_return->($local_response);
+        }
+        return $content;
+    }
     return $template->process( @_ );
 }

[abeverley]

Thanks @veryrusty - I agree it makes more sense to deal with the cause than the effect if possible. I was thinking that other template engines are probably less impacted, as I was assuming that the TT recursion checks are probably relatively unique, but definitely better to fix across all engines if possible. Look forward to hearing back once you've had time to refine your proposed solution.

[abeverley]

Hi @veryrusty - sorry to pester but just wondered whether you'd had any further thoughts on this?

[cromedome]

@veryrusty - any movement here? Thank you!

[abeverley]

@veryrusty - I wondered whether you'd had any thoughts on this? Just wondering whether I should patch my local Dancer in the short-term. Thanks!

[abeverley]

Hi @veryrusty sorry to bug again... I'd really like to get this fixed if possible, as it results in nasty intermittent bugs whereby a template gets rendered with the wrong content (which in the worst case can lead to data leakage and a security breach).

Could we maybe at least merge my patch in the meantime? (Maybe without the test, which I had the aforementioned problems with). Thanks!

[cromedome]

@abeverley I chatted with @veryrusty and I'm happy to merge this with his proposed change. If you could incorporate it in your PR that would be a huge help. I can get an update out this weekend that incorporates it. Seem workable?

[abeverley]

Brilliant, thanks to you both, will do!

Would you have any idea about the test problem I was having? (described in the opening description to this issue). I spent hours on it and got nowhere. It would be nice to have the test included, but as it is we would need to leave it out unfortunately.

[cromedome]

I'm unsure offhand, but will take a look as I am working with it this weekend.

[abeverley]

Thanks Jason. I've just been taking another look at it, but no further forward unfortunately. It seems to be something happening when objects are destroyed, as the error is happening after the last line of the test. It seems to work fine with either only the redirect in the test or only the template rendering, but having both causes the problem. If you have any ideas where to look then I am all ears!

[cromedome]

I took tests from @abeverley's PR, reversed the TT portion of the PR, and instead applied the patch suggested by @veryrusty in the discussion so that all template engines receive this fix. Tests pass, and code has been merged and pushed. Will do a release later today. Thanks for everyone's help on this one!

[abeverley]

Amazing, thanks @cromedome - I was wondering whether the tests would work on @veryrusty changes so pleased to hear they do and all is fixed! Looking forward to rolling this out. Many thanks to all for getting this sorted.