Middleware::ErrorDocument reuses Plack::Request fields from $env in subrequest

[pstuifzand]

If an errorcode, e.g. 404, is returned from $app and Plack::Request is used, then $app adds 'plack.request.' fields to the $env hash. If ErrorDocument now calls $app again (via subrequest) then the old fields are available in the Plack::Request of the second call to $app. The second Plack::Request shouldn't reuse plack.request.query. Deleting the plack.request fields from the $env hash solves this issue.

It however points to a bigger issue with ErrorDocument where the $env hash should possibly be copied.

Another problem exists in Plack::Request where it saves query information in plack.request.query, which is the wrong place, when using Plack::Request again in a second call using the same $env.

[miyagawa]

No, using plack.request.query as a cache to save parsed request query is a right thing to do and it's intentional that using Plack::Request twice with the same $env should result in the same query variables. It's by design, and intentionally done that way - stole from Rack's Rack.Request.

That said, I see the first problem, where $env is being reused in the ErrorDocument. Will see how we can fix it, but can I see the problematic code that you're having trouble with?

IMO this whole 'subrequest' concept in the ErrorDocument should be refactored/re-coded - it was written in the way early days, and could be implemented much better.

[pstuifzand]

I created a subclass on top of Plack::Request. When I use the class a second
time with the same env (in the subrequest) it remembers the previous controller.

In this code there are the problematic pieces of code. I expect that the query_parameters returns an empty object, instead of the object of the 'previous' request. This code adds 'controller' and 'action' variables from the new request to the query_parameters. If parameters is called before adding new values to query_parameters then body_parameters and query_parameters are already merged and parameters won't return the new values.

package WebWinkel::Web::Request;
use base 'Plack::Request';

sub parse_route {
    my ($self, $routes) = @_;
    my $route = $routes->test($self->uri->path);
    for (qw/controller action/) {
        die "Route doesn't contain a $_ value" unless defined $route->{$_};
    }
    for (keys %$route) {
        $self->query_parameters->add($_, $route->{$_});
    }
    return;
}

sub controller {
    my $self = shift;
    return $self->parameters->get_one('controller');
}

I removed some code, that's not important. I hope this helps.

[miyagawa]

Ok, so there are two problems: one is that the subrequest causes $env not to be cleaned up. It's actually intentional but Plack::Request and possibly other middleware that saves $env a cached value might get problematic.

The other thing is that you're trying to subclass Plack::Request and want to mess with parameters. I actually had this exact problem with Tatsumaki where it has its own subclassed Request.

I haven't yet found a good solution for this, but for the time being it probably might be a good idea to avoid reusing _parameters methods and instead define a new accessor that probably merges all the values from _parameters but do not rely on the $env storage cache.

It turns out the latest release and its patch revealed that there's a problem with HTTP::Body and its temporary files cleanup when we instantiate Plack::Request twice - kind of like the same problem, even though the subrequest case has its own intention, which is quite different from calling Plack::Request twice in a legit way (once in middleware and another in the app).