first pass at not streaming via the catalyst app, but instead allow the underlying...
John Napiorkowski [Sat, 21 Dec 2013 17:51:36 +0000 (11:51 -0600)]
Changes
lib/Catalyst/Delta.pod
lib/Catalyst/Engine.pm
lib/Catalyst/Response.pm
t/aggregate/live_engine_response_emptybody.t
t/body_fh.t [new file with mode: 0644]

diff --git a/Changes b/Changes
index bfc0053..036300d 100644 (file)
--- a/Changes
+++ b/Changes
@@ -3,6 +3,17 @@
   - 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.  
+
 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
index 8b5caf2..8281a0d 100755 (executable)
@@ -4,7 +4,41 @@ 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<Plack::Middleware::XSendfile> 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<Plack::Middleware::BufferedStreaming>
+or report specific problems to the dev team.
 
 =head2 VERSION 5.9XXXX 'cataplack'
 
index a5c177a..e2f77a2 100644 (file)
@@ -69,26 +69,70 @@ See L<Catalyst::Response/write> and L<Catalyst::Response/write_fh> 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;
 }
index 9c571b8..8c26ae4 100644 (file)
@@ -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;
 }
 
index beb83fe..e4f407c 100644 (file)
@@ -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 (file)
index 0000000..cc00020
--- /dev/null
@@ -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);