clustericious / Clustericious

2 stars 3 forks source link

app.t out of memory #12

Closed chorny closed 9 years ago

chorny commented 9 years ago

probably due to huge number of warnings Mojolicious::Controller::render_not_found is DEPRECATED in favor of the reply->not_found helper at C:/strawberry5161/perl/site/lib/Mojolicious/Renderer.pm line 76.

latest Mojolicious.

chorny commented 9 years ago

also

Deep recursion on subroutine "Clustericious::Controller::render_not_found" at /home/david/cpantesting/perl-5.18.1/lib/site_perl/5.18.1/Mojolicious/Plugin/DefaultHelpers.pm line 98.
Deep recursion on subroutine "Mojolicious::Renderer::Helpers::034c50630bc5e0fe5193592609242942::not_found" at /home/david/cpantesting/perl-5.18.1/.cpan/build/Clustericious-0.9941-wc96j4/blib/lib/Clustericious/Controller.pm line 58.
Deep recursion on anonymous subroutine at /home/david/cpantesting/perl-5.18.1/lib/site_perl/5.18.1/Mojolicious/Renderer.pm line 76.
Deep recursion on subroutine "Mojolicious::Plugin::DefaultHelpers::_development" at /home/david/cpantesting/perl-5.18.1/lib/site_perl/5.18.1/Mojolicious/Plugin/DefaultHelpers.pm line 42.

found in this report http://www.cpantesters.org/cpan/report/a822369c-a57c-11e4-a0ad-28cd227a829d

plicease commented 9 years ago

Thanks for opening a ticket; I have been meaning to look at these. Looks like mojo moved the implementation on render_not_found into a helper after deprecating it. hijinks ensue.

plicease commented 9 years ago

I'm tempted to go and remove the render_not_founds in the codebase and replace with the new form, but our version of this function is removing autodata from the stash:

sub render_not_found {
    my $self = shift;
    undef $self->stash->{autodata} if exists($self->stash->{autodata});
    $self->helpers->reply->not_found
}

@bduggan do you have any insight into why it is doing this I am pretty sure this usage predates me.

plicease commented 9 years ago

btw- render_exception has a similar problem, but that is there only for compatibility, so we can safely remove that provided that we go through and update all the places in the code base that are using it :P

bduggan commented 9 years ago

Clustericious::Plugin::AutodataHandler says that

If a route leaves data in stash->{autodata}, it is rendered by this handler, which chooses the type with the first acceptable type listed in the Accept header,

So, I think it must be related to that...but that said, I think your approach of removing it sounds good. The potential bug would be that a 404 response would also have a payload, maybe a hook could enure that this doesn't happen?

This effort also predates some of the more sophisticated handling of content negotation by Mojolicious (with respond_to etc). Making changes to Clustericious to take advantage of this might not be a bad idea.

plicease commented 9 years ago

Looks like we are good:

get '/autotest_not_found' => sub {
  my($self) = shift;
  $self->stash->{autodata} = [1,2,3];
  $self->reply->not_found;
};

...

$t->get_ok('/autotest_not_found')
  ->status_is(404);
note $t->tx->res->text;

tests as:

# DEBUG GET "/autotest_not_found"
# DEBUG Routing to a callback
# DEBUG Rendering template "not_found.development.html.ep" from DATA section
# DEBUG 404 Not Found (0.001369s, 730.460/s)
ok 20 - GET /autotest_not_found
ok 21 - 404 Not Found
# NOT FOUND :  "/autotest_not_found"

so I think we are good to just remove the deprecated methods.

bduggan commented 9 years ago

Since the autodata plugin only handles JSON and YAML (and x-ww-form-urlencoded) and varies the response based on the Accept header, I might send an Accept header for one of those types and see what's in the response body.

plicease commented 9 years ago

This should be fixed in 0.9942.