From: John Napiorkowski Date: Sun, 22 Dec 2013 16:22:43 +0000 (-0600) Subject: merged fix for regression from master X-Git-Tag: 5.90060~41 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=catagits%2FCatalyst-Runtime.git;a=commitdiff_plain;h=46aec47a553a7991d5a9e9faff176e9657777c44;hp=5bb2a5b3a3703b0eb12f6be0983e7f4676c55545 merged fix for regression from master --- diff --git a/Changes b/Changes index d903a0f..9318a45 100644 --- a/Changes +++ b/Changes @@ -1,5 +1,35 @@ # This file documents the revision history for Perl extension Catalyst. + - Announcing the repo is not open for development of Perl Catalyst 'Runner' + - http://questhub.io/realm/perl/explore/latest/tag/runner + +5.90059_002 - TBA + - We now pass a scalar or filehandle directly to you Plack handler, rather + than always use the streaming interface (we are still always using a + delayed response callback). This means that you can make use of Plack + middleware like Plack::Middleware::XSendfile and we expect better use of + server features (when they exist) like correct use of chunked encoding or + properly non blocking streaming when running under a supporting server like + Twiggy. See Catalyst::Delta for more. This change might cause issues if + you are making heaving use of streaming (although in general we expect things + to work much better. + - In the case when we remove a content body from the response because you set + an information status or a no content type status, warn that we are doing so + when in debug mode. You might see additional debugging information to help + you find and remove unneeded response bodies. + - Updated the code where Catalyst tries to guess a content length when you + fail to provide one. This should cause less issues when trying to guess the + length of a funky filehandle. This now uses Plack::Middleware::ContentLength + - Removed custom code to remove body content when the request is HEAD and + swapped it for Plack::Middleware::Head + - Merged fix for regressions from stable.. + +5.90059_001 - 2013-12-19 + - Removed deprecated Regexp dispatch type from dependency list. If you are + using Regex[p] type dispatching you need to add the standalone distribution + 'Catalyst::DispatchType::Regex' to you build system NOW or you application + will be broken. + 5.90053 - 2013-12-21 - Reverted a change in the previous release that moved the setup_log phase to after setup_config. This change was made to allow people to use diff --git a/Makefile.PL b/Makefile.PL index 33a73a6..3d06b69 100644 --- a/Makefile.PL +++ b/Makefile.PL @@ -76,10 +76,6 @@ requires 'Hash::MultiValue'; requires 'Plack::Request::Upload'; requires 'CGI::Struct'; -# Install the standalone Regex dispatch modules in order to ease the -# deprecation transition -requires 'Catalyst::DispatchType::Regex' => '5.90021'; - test_requires 'Test::Fatal'; test_requires 'Test::More' => '0.88'; test_requires 'Data::Dump'; diff --git a/lib/Catalyst.pm b/lib/Catalyst.pm index 1240a44..4e55662 100644 --- a/lib/Catalyst.pm +++ b/lib/Catalyst.pm @@ -41,6 +41,8 @@ use Plack::Middleware::ReverseProxy; use Plack::Middleware::IIS6ScriptNameFix; use Plack::Middleware::IIS7KeepAliveFix; use Plack::Middleware::LighttpdScriptNameFix; +use Plack::Middleware::ContentLength; +use Plack::Middleware::Head; use Plack::Util; use Class::Load 'load_class'; @@ -120,7 +122,7 @@ __PACKAGE__->stats_class('Catalyst::Stats'); # Remember to update this in Catalyst::Runtime as well! -our $VERSION = '5.90053'; +our $VERSION = '5.90059_002'; sub import { my ( $class, @arguments ) = @_; @@ -1857,11 +1859,6 @@ sub finalize { $c->finalize_headers unless $c->response->finalized_headers; - # HEAD request - if ( $c->request->method eq 'HEAD' ) { - $c->response->body(''); - } - $c->finalize_body; } @@ -1938,30 +1935,15 @@ EOF } } - # Content-Length - if ( defined $response->body && length $response->body && !$response->content_length ) { + # Remove incorrectly added body and content related meta data when returning + # an information response, or a response the is required to not include a body - # get the length from a filehandle - if ( blessed( $response->body ) && $response->body->can('read') || ref( $response->body ) eq 'GLOB' ) - { - my $size = -s $response->body; - if ( $size ) { - $response->content_length( $size ); - } - else { - $c->log->warn('Serving filehandle without a content-length'); - } - } - else { - # everything should be bytes at this point, but just in case - $response->content_length( length( $response->body ) ); - } - } - - # Errors if ( $response->status =~ /^(1\d\d|[23]04)$/ ) { - $response->headers->remove_header("Content-Length"); - $response->body(''); + if($response->has_body) { + $c->log->debug('Removing body for informational or no content http responses'); + $response->body(''); + $response->headers->remove_header("Content-Length"); + } } $c->finalize_cookies; @@ -3122,7 +3104,10 @@ L (which is now considered deprecated) sub registered_middlewares { my $class = shift; if(my $middleware = $class->_psgi_middleware) { - return @$middleware; + return ( + Plack::Middleware::ContentLength->new, + Plack::Middleware::Head->new, + @$middleware); } else { die "You cannot call ->registered_middlewares until middleware has been setup"; } diff --git a/lib/Catalyst/Delta.pod b/lib/Catalyst/Delta.pod index 985b571..22d3244 100755 --- a/lib/Catalyst/Delta.pod +++ b/lib/Catalyst/Delta.pod @@ -4,7 +4,44 @@ Catalyst::Delta - Overview of changes between versions of Catalyst =head1 DESCRIPTION -This is an overview of the user-visible changes to Catalyst between major Catalyst releases. +This is an overview of the user-visible changes to Catalyst between major +Catalyst releases. + +=head2 VERSION 5.90060+ + +We changed the way we return body content (from response) to whatever +Plack handler you are using (Starman, FastCGI, etc.) We no longer +always use the streaming interface for the cases when the body is a +simple scalar, object or filehandle like. In those cases we now just +pass the simple response on to the plack handler. This might lead to +some minor differences in how streaming is handled. For example, you +might notice that streaming starts using chunked encoding when running +on a server that supports that, or that previously missing headers +(possible content-length) might appear suddenly correct. Also, if you +are using middleware like L and are using +a filehandle that sets a readable path, your server might now correctly +handle the file (rather than as before where Catalyst would stream it +very likely very slowly). + +In other words, some things might be meaninglessly different and some +things that were broken codewise but worked because of Catalyst being +incorrect might suddenly be really broken. The behavior is now more +correct in that Catalyst plays better with features that Plack offers +but if you are making heavy use of the streaming interface there could +be some differences so you should test carefully (this is probably not +the vast majority of people). In particular if you are developing +using one server but deploying using a different one, differences in +what those server do with streaming should be noted. + +We also now more carefully distingush the different between a body set +to '' and a body that is undef. This might lead to situations where +again you'll get a content-length were you didn't get one before or +where a supporting server will start chunking output. If this is an +issue you can apply the middleware L +or report specific problems to the dev team. + +Also, we have started migrating code in Catalyst to equivilent Plack +Middleware when such exists and is correct to do so. =head2 VERSION 5.90053 diff --git a/lib/Catalyst/Engine.pm b/lib/Catalyst/Engine.pm index a5c177a..e2f77a2 100644 --- a/lib/Catalyst/Engine.pm +++ b/lib/Catalyst/Engine.pm @@ -69,26 +69,70 @@ See L and L for more. sub finalize_body { my ( $self, $c ) = @_; - return if $c->response->_has_write_fh; - - my $body = $c->response->body; - no warnings 'uninitialized'; - if ( blessed($body) && $body->can('read') or ref($body) eq 'GLOB' ) { - my $got; - do { - $got = read $body, my ($buffer), $CHUNKSIZE; - $got = 0 unless $self->write( $c, $buffer ); - } while $got > 0; - - close $body; - } - else { - $self->write( $c, $body ); - } + my $res = $c->response; # We use this all over + + ## If we've asked for the write 'filehandle' that means the application is + ## doing something custom and is expected to close the response + return if $res->_has_write_fh; + + if($res->_has_response_cb) { + ## we have not called the response callback yet, so we are safe to send + ## the whole body to PSGI + + my @headers; + $res->headers->scan(sub { push @headers, @_ }); + + ## We need to figure out what kind of body we have... + my $body = $res->body; + if(defined $body) { + if(blessed($body) && $body->can('read') or ref($body) eq 'GLOB') { + # Body is a filehandle like thingy. We can jusrt send this along + # to plack without changing it. + } else { + # Looks like for backcompat reasons we need to be able to deal + # with stringyfiable objects. + $body = "$body" if blessed($body); # Assume there's some sort of overloading.. + $body = [$body]; + } + } else { + $body = [undef]; + } + + $res->_response_cb->([ $res->status, \@headers, $body]); + $res->_clear_response_cb; + + } else { + ## Now, if there's no response callback anymore, that means someone has + ## called ->write in order to stream 'some stuff along the way'. I think + ## for backcompat we still need to handle a ->body. I guess I could see + ## someone calling ->write to presend some stuff, and then doing the rest + ## via ->body, like in a template. + + ## We'll just use the old, existing code for this (or most of it) + + if(my $body = $res->body) { + no warnings 'uninitialized'; + if ( blessed($body) && $body->can('read') or ref($body) eq 'GLOB' ) { + + ## In this case we have no choice and will fall back on the old + ## manual streaming stuff. + + my $got; + do { + $got = read $body, my ($buffer), $CHUNKSIZE; + $got = 0 unless $self->write($c, $buffer ); + } while $got > 0; + + close $body; + } + else { + $self->write($c, $body ); + } + } - my $res = $c->response; - $res->_writer->close; - $res->_clear_writer; + $res->_writer->close; + $res->_clear_writer; + } return; } diff --git a/lib/Catalyst/Response.pm b/lib/Catalyst/Response.pm index 9c571b8..8c26ae4 100644 --- a/lib/Catalyst/Response.pm +++ b/lib/Catalyst/Response.pm @@ -9,7 +9,7 @@ with 'MooseX::Emulate::Class::Accessor::Fast'; has _response_cb => ( is => 'ro', - isa => 'CodeRef', + isa => 'CodeRef', writer => '_set_response_cb', clearer => '_clear_response_cb', predicate => '_has_response_cb', @@ -20,12 +20,30 @@ subtype 'Catalyst::Engine::Types::Writer', has _writer => ( is => 'ro', - isa => 'Catalyst::Engine::Types::Writer', - writer => '_set_writer', + isa => 'Catalyst::Engine::Types::Writer', #Pointless since we control how this is built + #writer => '_set_writer', Now that its lazy I think this is safe to remove clearer => '_clear_writer', predicate => '_has_writer', + lazy => 1, + builder => '_build_writer', ); +sub _build_writer { + my $self = shift; + + ## These two lines are probably crap now... + $self->_context->finalize_headers unless + $self->finalized_headers; + + my @headers; + $self->headers->scan(sub { push @headers, @_ }); + + my $writer = $self->_response_cb->([ $self->status, \@headers ]); + $self->_clear_response_cb; + + return $writer; +} + has write_fh => ( is=>'ro', predicate=>'_has_write_fh', @@ -33,12 +51,7 @@ has write_fh => ( builder=>'_build_write_fh', ); -sub _build_write_fh { - my $self = shift; - $self->_context->finalize_headers unless - $self->finalized_headers; - $self->_writer; -}; +sub _build_write_fh { shift ->_writer } sub DEMOLISH { my $self = shift; @@ -89,26 +102,6 @@ sub write { sub finalize_headers { my ($self) = @_; - - # This is a less-than-pretty hack to avoid breaking the old - # Catalyst::Engine::PSGI. 5.9 Catalyst::Engine sets a response_cb and - # expects us to pass headers to it here, whereas Catalyst::Enngine::PSGI - # just pulls the headers out of $ctx->response in its run method and never - # sets response_cb. So take the lack of a response_cb as a sign that we - # don't need to set the headers. - - return unless $self->_has_response_cb; - - # If we already have a writer, we already did this, so don't do it again - return if $self->_has_writer; - - my @headers; - $self->headers->scan(sub { push @headers, @_ }); - - my $writer = $self->_response_cb->([ $self->status, \@headers ]); - $self->_set_writer($writer); - $self->_clear_response_cb; - return; } diff --git a/lib/Catalyst/Runtime.pm b/lib/Catalyst/Runtime.pm index 677e547..d98fc06 100644 --- a/lib/Catalyst/Runtime.pm +++ b/lib/Catalyst/Runtime.pm @@ -7,7 +7,7 @@ BEGIN { require 5.008003; } # Remember to update this in Catalyst as well! -our $VERSION = '5.90053'; +our $VERSION = '5.90059_002'; =head1 NAME diff --git a/lib/Catalyst/Upgrading.pod b/lib/Catalyst/Upgrading.pod index 03a24ab..11d0198 100644 --- a/lib/Catalyst/Upgrading.pod +++ b/lib/Catalyst/Upgrading.pod @@ -2,6 +2,18 @@ Catalyst::Upgrading - Instructions for upgrading to the latest Catalyst +=head1 Upgrading to Catalyst 5.90060 + +Starting in the v5.90059_001 development release, the regexp dispatch type is +no longer automatically included as a dependency. If you are still using this +dispatch type, you need to add L into your build +system. + +The standalone distribution of Regexp will be supported for the time being, but +should we find that supporting it prevents us from moving L forward +in necessary ways, we reserve the right to drop that support. It is highly +recommended that you use this last stage of deprecation to change your code. + =head1 Upgrading to Catalyst 5.90040 =head2 Catalyst::Plugin::Unicode::Encoding is now core diff --git a/t/aggregate/live_engine_response_emptybody.t b/t/aggregate/live_engine_response_emptybody.t index beb83fe..e4f407c 100644 --- a/t/aggregate/live_engine_response_emptybody.t +++ b/t/aggregate/live_engine_response_emptybody.t @@ -18,7 +18,11 @@ use Catalyst::Test 'TestApp'; { my $res = request('/emptybody'); is $res->content, ''; - ok !defined $res->header('Content-Length'); + + SKIP: { + skip "content-length for body of '' is now server dependent", 1; + ok !defined $res->header('Content-Length'); + } } done_testing; diff --git a/t/body_fh.t b/t/body_fh.t new file mode 100644 index 0000000..cc00020 --- /dev/null +++ b/t/body_fh.t @@ -0,0 +1,123 @@ +use warnings; +use strict; +use Test::More; +use HTTP::Request::Common; +use HTTP::Message::PSGI; +use Plack::Util; +use Devel::Dwarn; + +{ + package MyApp::Controller::Root; + + use base 'Catalyst::Controller'; + + sub flat_response :Local { + my $response = 'Hello flat_response'; + pop->res->body($response); + } + + sub memory_stream :Local { + my $response = 'Hello memory_stream'; + open my $fh, '<', \$response || die "$!"; + + pop->res->body($fh); + } + + sub manual_write_fh :Local { + my ($self, $c) = @_; + my $response = 'Hello manual_write_fh'; + my $writer = $c->res->write_fh; + $writer->write($response); + $writer->close; + } + + sub manual_write :Local { + my ($self, $c) = @_; + $c->res->write('Hello'); + $c->res->body('manual_write'); + } + + package MyApp; + use Catalyst; + +} + +$INC{'MyApp/Controller/Root.pm'} = '1'; # sorry... + +ok(MyApp->setup); +ok(my $psgi = MyApp->psgi_app); + +{ + ok(my $env = req_to_psgi(GET '/root/flat_response')); + ok(my $psgi_response = $psgi->($env)); + + $psgi_response->(sub { + my $response_tuple = shift; + my ($status, $headers, $body) = @$response_tuple; + + ok $status; + ok $headers; + is $body->[0], 'Hello flat_response'; + + }); +} + +{ + ok(my $env = req_to_psgi(GET '/root/memory_stream')); + ok(my $psgi_response = $psgi->($env)); + + $psgi_response->(sub { + my $response_tuple = shift; + my ($status, $headers, $body) = @$response_tuple; + + ok $status; + ok $headers; + is ref($body), 'GLOB'; + + }); +} + +{ + ok(my $env = req_to_psgi(GET '/root/manual_write_fh')); + ok(my $psgi_response = $psgi->($env)); + + $psgi_response->(sub { + my $response_tuple = shift; + my ($status, $headers, $body) = @$response_tuple; + + ok $status; + ok $headers; + ok !$body; + + return Plack::Util::inline_object( + write => sub { is shift, 'Hello manual_write_fh' }, + close => sub { ok 1, 'closed' }, + ); + }); +} + +{ + ok(my $env = req_to_psgi(GET '/root/manual_write')); + ok(my $psgi_response = $psgi->($env)); + + $psgi_response->(sub { + my $response_tuple = shift; + my ($status, $headers, $body) = @$response_tuple; + + ok $status; + ok $headers; + ok !$body; + + my @expected = (qw/Hello manual_write/); + return Plack::Util::inline_object( + close => sub { ok 1, 'closed'; is scalar(@expected), 0; }, + write => sub { is shift, shift(@expected) }, + ); + }); +} + +## We need to specify the number of expected tests because tests that live +## in the callbacks might never get run (thus all ran tests pass but not all +## required tests run). + +done_testing(28);