POST requests with a large body
redneb
execute the CGI script right after it has received all headers. The
body of the request is then "streamed" to the CGI script as it is
being received by the server without having to store it in the disk or
in memory.
The PSGI specification doesn't specify whether the PSGI app is
executed after all headers have been received or after the entire body
has been received.
I experimented with starman, and it seems that it stores the message
body in a file and when the entire body has been received, it calls
the sub from the PSGI app. This of course can cause problems, eg a
POST request with a really large body can use all disk space. How is a
PSGI app supposed to handle this case?
Tatsuhiko Miyagawa
> When a POST request is being sent to CGI script, the server will
It depends on which web server you're running with which configuration.
> The PSGI specification doesn't specify whether the PSGI app is
> executed after all headers have been received or after the entire body
> has been received.
You're right that it doesn't specify the "right" behavior. it's up to
the server implementations.
> I experimented with starman, and it seems that it stores the message
> body in a file and when the entire body has been received, it calls
> the sub from the PSGI app. This of course can cause problems, eg a
> POST request with a really large body can use all disk space.
IMO it's much better than saving the large body on memory, and the way
your application is not kicked until the upload is done is more
efficient in terms of process management but YMMV. I could see how
useful it'd be if you can hook into the mime header declaration phase
in Starman, for instance, to stop processing at all. But i guess
that's still out of the PSGI scope.
> How is a PSGI app supposed to handle this case?
PSGI 1.1 specifies the optional parameter in the environment called
psgix.input.buffered which is a boolean value that indicates whether
the request body is buffered by the server or not. However your
application normally doesn't need to change the behavior - psgi.input
behaves as an input filehandle in any ways.
The problem with the streaming mode (i.e. psgix.input.buffered is
really false) is that your application needs to handle the
non-blocking read case and does proper retries, which is not necessary
in the case of buffered input.
I experimented the non-buffered upload with the built-in
HTTP::Server::PSGI and abandoned the idea for the sake of simplicity.
The unmerged commits are on
http://github.com/miyagawa/Plack/tree/topic/chunked-input (note that
the branch has the name about chunked but it also deals with
non-chunked streaming input as well)
If you really like to handle uploads in the streaming fashion you
probably have to write your own web server that does that. I guess it
might be useful to have a proper hook system in servers like Starman
to deal with it without a complete rewrite but i don't have a tuit to
do it yet.
HTH
--
Tatsuhiko Miyagawa
redneb
io can be a problem. But Starman uses blocking io.
I tried to implement that in Starman and you can see the results in
the patch at the end of this message. The basic idea is to introduce a
new class (Plack::InputStream in the code below) that can be used as
the $env->{'psgi.input'}. Plack::InputStream wraps around the
underlying tcp socket in order to decode the input stream on the fly
if necessary (i.e. in the case of chunked encoding) as well as to make
sure that the app does not read past the body of the current request.
If the app does not "consume" the entire input stream then the server
makes sure that this happens after the app has returned.
I think this approach has several advantages:
- it is more elegant
- it gives the developer more flexibility
- it does not violate the PSGI specification
- it should be faster as it does not involve any disk io
- it is more secure (with the buffered approach you can run out of
memory or disk space, also the POST data are not written to the disk
so the developer does not have to worry about sensitive data being
written to the disk)
- it has lower complexity than the buffered approach (if you look the
code below, Starman::Server is more simple, some parts of it are moved
to Plack::InputStream module)
The code below is just a proof of concept. I haven't done extensive
testing yet. Please let me know if you find this interesting at all.
----------------------------------------
diff -ruN a/lib/perl5/Plack/InputStream.pm b/lib/perl5/Plack/
InputStream.pm
--- a/lib/perl5/Plack/InputStream.pm 1969-12-31 19:00:00.000000000
-0500
+++ b/lib/perl5/Plack/InputStream.pm 2010-08-12 03:03:44.913087387
-0400
@@ -0,0 +1,87 @@
+package Plack::InputStream;
+use strict;
+use warnings;
+
+use constant CHUNKSIZE => 64 * 1024;
+
+sub new {
+ my($class, $socket, $length, $inputbuf) = @_;
+ # if length is defined then content-length has been specified
+ # if length is undef then chunked transfer encoding is being used
+
+ $inputbuf = '' unless defined $inputbuf;
+
+ bless { socket => $socket, length => $length, inputbuf =>
$inputbuf }, $class;
+}
+
+sub read {
+ my($self, $_buf, $len, $offset) = @_;
+ $offset ||= 0;
+ $_[1] = '' unless defined $_[1];
+ if ($self->{EOF}) {
+ return 0;
+ } elsif (defined $self->{length}) {
+ # content-length has been specified
+ $len = $self->{length} if $len > $self->{length};
+ if (my $buflen = length $self->{inputbuf}) {
+ $len = $buflen if $len > $buflen;
+ substr $_[1], $offset, $len, (substr $self->{inputbuf},
0, $len, '');
+ } else {
+ my $len = sysread $self->{socket}, $_[1], $len, $offset;
+ return unless defined $len; # io error, just return undef
+ }
+ $self->{length} -= $len;
+ return if $len == 0 && $self->{length} != 0; # return undef,
connection closed before the entire body was read
+ $self->{EOF} = 1 if $self->{length} == 0;
+ return $len;
+ } else {
+ # chunked transfer encoding
+ while (1) {
+ if ( $self->{inputbuf} =~ /^(([0-9a-fA-F]+).*\015\012)/ )
{
+ my $chunk_len = hex $2;
+ my $trailer_len = length $1;
+ my $gross_len = $trailer_len + $chunk_len + 2;
+
+ if (length $self->{inputbuf} >= $gross_len) {
+ # we have a complete chunk, we can now extract it
from the inputbuf
+ my $chunk = substr $self->{inputbuf},
$trailer_len, $chunk_len;
+ substr $self->{inputbuf}, 0, $gross_len, '';
+
+ if ($chunk_len == 0) {
+ $self->{EOF} = 1;
+ return 0;
+ } else {
+ $len = $chunk_len if $len > $chunk_len;
+ substr $_[1], $offset, $len, (substr $chunk,
0, $len, '');
+
+ if (my $ch_l = length $chunk) {
+ $self->{inputbuf} = sprintf( "%x",
$ch_l ) . "\r\n" . $chunk . "\r\n" . $self->{inputbuf};
+ }
+
+ return $len;
+ }
+ }
+
+ }
+ my $read = sysread $self->{socket}, my($data), CHUNKSIZE;
+ return unless defined $read;
+ $self->{inputbuf} .= $data;
+ }
+ }
+}
+
+sub finalize {
+ my $self = shift;
+
+ # consume what's left
+ my $buf='';
+ while (1) {
+ my $read = $self->read($buf, CHUNKSIZE);
+ die "Read error\n" unless defined $read;
+ last if $read == 0;
+ }
+
+ return $self->{inputbuf};
+}
+
+1;
diff -ruN a/lib/perl5/Starman/Server.pm b/lib/perl5/Starman/Server.pm
--- a/lib/perl5/Starman/Server.pm 2010-07-02 20:20:10.000000000 -0400
+++ b/lib/perl5/Starman/Server.pm 2010-08-12 00:04:47.570691471 -0400
@@ -11,7 +11,7 @@
use Symbol;
use Plack::Util;
-use Plack::TempBuffer;
+use Plack::InputStream;
use constant DEBUG => $ENV{STARMAN_DEBUG} || 0;
use constant CHUNKSIZE => 64 * 1024;
@@ -151,7 +151,7 @@
'psgi.multithread' => Plack::Util::FALSE,
'psgi.multiprocess' => Plack::Util::TRUE,
'psgix.io' => $conn,
- 'psgix.input.buffered' => Plack::Util::TRUE,
+ 'psgix.input.buffered' => Plack::Util::FALSE,
};
# Parse headers
@@ -216,6 +216,11 @@
# Run PSGI apps
my $res = Plack::Util::run_app($self->{app}, $env);
+ # make sure that all of the message body has been read
+ if ( ref $env->{'psgi.input'} eq 'Plack::InputStream' ) {
+ $self->{client}->{inputbuf} = $env->{'psgi.input'}-
>finalize
+ }
+
if (ref $res eq 'CODE') {
$res->(sub { $self->_finalize_response($env, $_[0]) });
} else {
@@ -328,62 +333,12 @@
sub _prepare_env {
my($self, $env) = @_;
- my $get_chunk = sub {
- if ($self->{client}->{inputbuf}) {
- my $chunk = delete $self->{client}->{inputbuf};
- return ($chunk, length $chunk);
- }
- my $read = sysread $self->{server}->{client}, my($chunk),
CHUNKSIZE;
- return ($chunk, $read);
- };
-
my $chunked = do { no warnings; lc delete $env-
>{HTTP_TRANSFER_ENCODING} eq 'chunked' };
if (my $cl = $env->{CONTENT_LENGTH}) {
- my $buf = Plack::TempBuffer->new($cl);
- while ($cl > 0) {
- my($chunk, $read) = $get_chunk->();
-
- if ( !defined $read || $read == 0 ) {
- die "Read error: $!\n";
- }
-
- $cl -= $read;
- $buf->print($chunk);
- }
- $env->{'psgi.input'} = $buf->rewind;
+ $env->{'psgi.input'} = Plack::InputStream->new($self-
>{server}->{client}, $cl, (delete $self->{client}->{inputbuf}));
} elsif ($chunked) {
- my $buf = Plack::TempBuffer->new;
- my $chunk_buffer = '';
- my $length;
-
- DECHUNK:
- while (1) {
- my($chunk, $read) = $get_chunk->();
- $chunk_buffer .= $chunk;
-
- while ( $chunk_buffer =~ s/^(([0-9a-fA-F]+).*
\015\012)// ) {
- my $trailer = $1;
- my $chunk_len = hex $2;
-
- if ($chunk_len == 0) {
- last DECHUNK;
- } elsif (length $chunk_buffer < $chunk_len) {
- $chunk_buffer = $trailer . $chunk_buffer;
- last;
- }
-
- $buf->print(substr $chunk_buffer, 0, $chunk_len, '');
- $chunk_buffer =~ s/^\015\012//;
-
- $length += $chunk_len;
- }
-
- last unless $read && $read > 0;
- }
-
- $env->{CONTENT_LENGTH} = $length;
- $env->{'psgi.input'} = $buf->rewind;
+ $env->{'psgi.input'} = Plack::InputStream->new($self-
>{server}->{client}, undef, (delete $self->{client}->{inputbuf}));
} else {
$env->{'psgi.input'} = $null_io;
}
Tatsuhiko Miyagawa
> I tried to implement that in Starman and you can see the results in
> the patch at the end of this message. The basic idea is to introduce a
> new class (Plack::InputStream in the code below) that can be used as
> the $env->{'psgi.input'}.
I think it's quite similar to what I've done a while ago with this:
http://github.com/miyagawa/Plack/commit/55eb32c738c67e34630c65bfa4ceb59b6365cdf5
> I think this approach has several advantages:
> - it is more elegant
> - it gives the developer more flexibility
> - it does not violate the PSGI specification
> - it should be faster as it does not involve any disk io
> - it is more secure (with the buffered approach you can run out of
> memory or disk space, also the POST data are not written to the disk
> so the developer does not have to worry about sensitive data being
> written to the disk)
> - it has lower complexity than the buffered approach (if you look the
> code below, Starman::Server is more simple, some parts of it are moved
> to Plack::InputStream module)
I haven't tested it either but if I remember it right this kind of
approach breaks the keep-alive and HTTP/1.1 pipelining since the
application can now finish before actually reading the input. Oh yeah,
your code has nasty check to call ->finalize on this new class.
I had a note about why this unbuffered read sounds like a good idea
but actually turned out making things complicated - it should be
somewhere on my blog, irc or on github commit logs but can't find it
now..
--
Tatsuhiko Miyagawa
Tatsuhiko Miyagawa
> I had a note about why this unbuffered read sounds like a good idea
> but actually turned out making things complicated - it should be
> somewhere on my blog, irc or on github commit logs but can't find it
> now..
Ah, there you go: http://gist.github.com/295653
--
Tatsuhiko Miyagawa
redneb
cases. My claim is that it is a better approach for Starman.
Starman uses blocking io. It already runs a loop like the following:
while ($env->{'psgi.input'}->read($buf, $length)) {
...
}
If there is any problem with this loop, Starman will be affected by
that problem _anyway_, no matter if the loop is run by the server or
by the app. Furthermore, I think the EWOULDBLOCK issue applies only to
non-blocking io, I have never encountered this problem with blocking
io on a socket. But even if it does, as I explained, Starman will be
affected anyway.
Regarding the issue that
> [..] when encoutering such an error but that would
> block the entire server for the request [...]
Well, again, Starman runs the above loop anyway. If something would
block the entire server, that would happen _anyway_, no matter if the
loop is run by the server or by the app.
In conclusion, I honestly don't see how the proposed change can make
things worse. It does not introduce any new problems. And let me
reiterate that I agree with you that the streaming approach is not the
optimal one for all cases, and this is why it should not be
implemented in Plack, as you tried to do in the past, but it could be
implemented in Starman instead.
Tatsuhiko Miyagawa
> Regarding the issue that
>
>> [..] when encoutering such an error but that would
>> block the entire server for the request [...]
>
> Well, again, Starman runs the above loop anyway.
I was talking about the loop in the application code i.e. the code in
Plack::Request as well as PSGI handlers of web application frameworks.
> In conclusion, I honestly don't see how the proposed change can make
> things worse. It does not introduce any new problems. And let me
> reiterate that I agree with you that the streaming approach is not the
> optimal one for all cases,
Yep, that being said i think this change that is only applicable to
Starman kind of makes sense. I still do not like the idea of blessing
the filehandle into a new class and do some kind of special-casing to
ensure the read, though.
> and this is why it should not be
> implemented in Plack, as you tried to do in the past, but it could be
> implemented in Starman instead.
Not really - my original take was to do it in HTTP::Server::PSGI which
*happens to* be bundled with Plack and is the default, but it's
actually just one of the PSGI server implementations in the wild.
--
Tatsuhiko Miyagawa
Tatsuhiko Miyagawa
> execute the CGI script right after it has received all headers. The
> body of the request is then "streamed" to the CGI script as it is
> being received by the server without having to store it in the disk or
> in memory.
FWIW, Apache 2.2.14 with mod_cgi buffers the POST input before
executing the cgi script. Confirmed with the stock Apache that comes
with OS X and a telnet session.
--
Tatsuhiko Miyagawa
redneb
> FWIW, Apache 2.2.14 with mod_cgi buffers the POST input before
> executing the cgi script. Confirmed with the stock Apache that comes
> with OS X and a telnet session.
Once it has received 1024 bytes, it will send them to the cgi script.
OTOH, it does buffer the entire output stream, but this is a
completely different story.
On Aug 12, 3:49 pm, Tatsuhiko Miyagawa <miyag...@gmail.com> wrote:
> I was talking about the loop in the application code i.e. the code in
> Plack::Request as well as PSGI handlers of web application frameworks.
Plack::Request->content does a $fh->seek(0, 0) which would not be
possible with the streaming approach. BTW, I think rewinding the input
stream is a bad idea. The PSGI specification clearly says that seek is
an optional method, Plack::Request should not rely on seek.
Furthermore, I think it would be more natural if Plack::Request-
>content (or raw_body) consumed the entrire input stream. If I am not
mistaken, that's what CGI.pm does.
> Yep, that being said i think this change that is only applicable to
> Starman kind of makes sense. I still do not like the idea of blessing
> the filehandle into a new class and do some kind of special-casing to
> ensure the read, though.
then we can create 2 classes, one for "Content-Encoding: identity" and
another one for "Content-Encoding: chunked".
Tatsuhiko Miyagawa
> It does not buffer the entire body. It has a buffer of 1024 bytes.
> Once it has received 1024 bytes, it will send them to the cgi script.
> OTOH, it does buffer the entire output stream, but this is a
> completely different story.
*nods*
>> I was talking about the loop in the application code i.e. the code in
>> Plack::Request as well as PSGI handlers of web application frameworks.
>
> Maybe I am missing something, but the only problem I see, is that
> Plack::Request->content does a $fh->seek(0, 0) which would not be
> possible with the streaming approach. BTW, I think rewinding the input
> stream is a bad idea. The PSGI specification clearly says that seek is
> an optional method, Plack::Request should not rely on seek.
You're missing the point of the code - it does ->seek() *after*
buffering the whole input in _parse_request_body where it swaps the
psgi.input to the buffered input stream. If the server already does
the buffering already, the psgix.input.bufferred flag is enabled so
the while loop to buffer the entire body is skipped, obviously.
> Furthermore, I think it would be more natural if Plack::Request-
>>content (or raw_body) consumed the entrire input stream. If I am not
> mistaken, that's what CGI.pm does.
Yes, $req->content does the entire input stream buffering when called.
Plack::Request's complicated logic is to deal with the case when one
of the middleware component uses Plack::Request to read POST body
while the actual application framework doesn't use Plack::Request and
reads from $env->{'psgi.input'} again - that's why we need to reset
and rewind the stream.
That said, Plack::Request should work with your proposed patch.
--
Tatsuhiko Miyagawa
Tatsuhiko Miyagawa
> Maybe I am missing something, but the only problem I see, is that
> Plack::Request->content does a $fh->seek(0, 0) which would not be
> possible with the streaming approach. BTW, I think rewinding the input
> stream is a bad idea. The PSGI specification clearly says that seek is
> an optional method, Plack::Request should not rely on seek.
http://github.com/miyagawa/psgi-specs/commit/1641d76c8e7ecce3f939ad8d431d1c06799172da
is the change for PSGI specification i made back in May to state that
->seek() should be safely called when (and only when)
psgix.input.buffered is set to true. This version is in the work to
reflect PSGI 1.0 -> 1.1 changes and has not been available on CPAN
yet.
--
Tatsuhiko Miyagawa
redneb
accept my patch. Would you at least consider adding a --max-post-
size=xxxx command line option? Although I this is not the most elegant
solution, it will mitigate the running-out-of-disk-space problem. And
speaking of size, you could also add a check about the total size of
headers in Starman::Server::_read_headers. This way $self->{client}-
>{inputbuf} will not grow arbitrarily large if someone tries to send 1
GB of headers. If you don't have time to implement these changes, I
would be happy to help you.
On Aug 12, 7:05 pm, Tatsuhiko Miyagawa <miyag...@gmail.com> wrote:
> You're missing the point of the code - it does ->seek() *after*
> buffering the whole input in _parse_request_body where it swaps the
> psgi.input to the buffered input stream. If the server already does
> the buffering already, the psgix.input.bufferred flag is enabled so
> the while loop to buffer the entire body is skipped, obviously.
work BTW, the whole PSGI/Plack think is brilliant!
Tatsuhiko Miyagawa
> Well, no matter how hard I try, it seems that I cannot convince you to
> accept my patch.
Hm what? I thought we've been discussing the impact of this patch, and
i think i've successfully spotted the point you might have overlooked,
like the way Plack::Request is implemented and how PSGI 1.1 introduces
the new buffered flag.
And look at my last email saying Plack::Request should work fine with
your proposed patch.
> Would you at least consider adding a --max-post-
> size=xxxx command line option? Although I this is not the most elegant
> solution, it will mitigate the running-out-of-disk-space problem. And
> speaking of size, you could also add a check about the total size of
> headers in Starman::Server::_read_headers. This way $self->{client}-
>>{inputbuf} will not grow arbitrarily large if someone tries to send 1
> GB of headers. If you don't have time to implement these changes, I
> would be happy to help you.
I think adding post size limit makes sense, as well as the ability to
stream the POST body like your original patch does. My thoughts are:
* the buffer size limit should probably be configurable, so the
smaller POST body should be just buffered in the server before running
the app.
* Plack::InputStream is not a good name for the module.
But at the same time, Starman is supposed to run behind the frontend
proxy most of the time and I think limiting the size should be easily
done in the frontend like with nginx, lighttpd or mod_proxy.
--
Tatsuhiko Miyagawa
red...@gmx.com
>I think adding post size limit makes sense, as well as the ability to
>stream the POST body like your original patch does.
I think it doesn't make sense to have both. Suppose that a POST request
is being received by a server that uses the streaming approach and that
the request has chunked encoding. Then the server cannot know in advance
whether the body size is going to exceed a given limit.
For me the ideal implementation for Starman would be like this:
The server should always stream the body without any buffering and it
should not offer any other option. And then we create a middleware that
buffers the entire body in a Plack::TempBuffer object and offers that as
the $env->{'psgi.input'} to the PSGI app (just like the current behavior
of Starman). Additionally, the middleware could offer an option to
limit the size of the body of the request.
>* Plack::InputStream is not a good name for the module.
I agree. And what's even more ugly is the fact that in my patch you have
to pass $self->{client}->{inputbuf} to the Plack::InputStream object and
then take it back when you call finalize.
Tatsuhiko Miyagawa
Dechunk plack middleware.
--
Tatsuhiko Miyagawa
Tatsuhiko Miyagawa
> I tried to implement that in Starman and you can see the results in
> the patch at the end of this message.
I'm trying to incorporate this patch into an experimental branch to
see how it performs and works with the existing apps, but the patch
seems malformed because of your mailer. Can you post this patch to a
nopaste site, or fork on github so that I can easily pull?
Thanks,
--
Tatsuhiko Miyagawa
red...@gmx.com
the following 2 issues in the first version:
* Avoid passing $self->{client}->{inputbuf} around. This is ugly and
slow.
* Avoid duplicate code in reading the HTTP headers and decoding chunked
transfer encoding. These 2 cases are similar; you have to read from the
socket until a certain regex is satisfied. This is why we need a
inputbuf in the first place. Otherwise doing sysreads without any
buffering would suffice.
My solution for this 2 issues is to wrap the socket into a new class
Starman::ReadSocket that abstracts the handing of inputbuf and can be
used for the headers parsing as well as the dechunking process. It
offers to methods: read and readrecord. read is like sysread.
readrecord is like readline with 2 wrinkles: it uses a regex for EOL and
it has a maximum length restiction in order to prevent buffer overflow
attacks.
This approach greatly simplifies some parts in lib/Starman/Server.pm:
_read_headers & _prepare_env are reduced down to trivial size or
eliminated altogether and lines 227-255 are eliminated too.
I tried to minimize the performance impact of this additional IO layer
by avoiding copying the buffer. I run a simple benchmark using a simple
hello world PSGI app and it runs 3-5% percent faster when doing GET &
POST requests (with small body). I expect even greater gains on POST
requests with bigger bodies.
I have uploaded the new patch here: http://gist.github.com/558192
Tatsuhiko Miyagawa
So the problem i found so far is the compatibility with HTTP::Body,
which is used by frameworks as well as Plack::Request. If given undef
$content_length HTTP::Body thinks the input stream is chunked, which
would cause a conflict because in our case the server (Starman)
dechunks for the app.
This behavior is compatible to what mod_cgi does (STDIN is dechunked
if Transfer-Encoding is chunked), so I believe it's a good change in
the long term, but:
a) existing apps that rely on HTTP::Body's auto dechunk would break -
we need to fix the code, or HTTP::Body to skip the dechunk code
(without mangling the 'chunked' attribute that has no setter right
now)
b) Even if we change the app to assume the input stream is dechunked,
then that app won't work with other web servers that would NOT
dechunk. We might need an indicator in the $env (like
psgix.input.dechunked?) so that to see read($env->{'psgi.input'})
would return the raw chunked stream or dechunked stream.
What do you think?
--
Tatsuhiko Miyagawa
red...@gmx.com
You are right, my patch is incompatible with HTTP::Body in the case of
chunked encoding. But I think HTTP::Body is to blame for that, not my
patch. In fact, it seems that the issue of POST request with chunked
body is more complicated than I initially thought.
I did some experiments today and here are the results:
1) Many web servers (e.g. nginx, lighttpd, older versions of IIS) do not
accept POST requests with chunked body at all, they reject them with
"411 Length Required" and then they close the connection. This is not
directly related to our situation but I thought it was interesting.
BTW, you can download the script I created for this experiment from
here: http://gist.github.com/559889.
2) The CGI spec does not specify what to do with POST requests with
chunked body. In apache, mod_cgi does decode chunked bodies but it does
not remove the Transfer-Encoding header and of course it does not add a
Content-Length header (since it does not buffer the entire body). On the
other hand, apache/mod_fcgid does decode chunked bodies too, but it also
removes the Transfer-Encoding header (and of course it does not add a
Content-Length header). I did not have time to test mod_perl.
So how can we do to improve the current situation? He are some thoughts:
A) The PSGI spec must be amended to avoid any possible ambiguity related
to this issue. The fact that the CGI spec doesn't say what to do in this
case causes many problems and we should not repeat the same mistake with
the PSGI spec.
So what should we do? From a theoretical point of view, a PSGI has 4
possibilities regarding chunked bodies:
a) Reject such requests with "411 Length Required" as soon as all
request headers have been received. Then the connection would be closed
(specifying "Connection: close" in the response headers) without doing
any parsing of the body. That's exactly like case 1 above.
b) Decode the body and remove the "Transfer-Encoding" header.
c) Decode the body and keep the "Transfer-Encoding" header (just like
apache/mod_cgi).
d) Leave the "Transfer-Encoding" header and the body intact and let the
PSGI app to do all the work.
I believe that options c & d don't make much sense and should be
disallowed by the PSGI spec. Option c is clearly inelegant. Regarding
option d: if a HTTP/1.1 server supports keep-alive connections then it
must be able to recognize chunked bodies in order to be able to process
subsequent pipelined requests. But in that case, it might as well decode
the body since it will have to parse it anyway.
B) The PSGI spec should clearly indicate that psgi.input cannot be used
to read past the end of the body.
C) We must fix HTTP::Body. Unfortunately this cannot be done without an
api change. Its constructor should accept an extra argument used to
indicated chunked encoding. Additionally, there must be some mechanism
to inform the HTTP::Body object that we have reached the end of the body
because in the case of decoded bodies with unspecified length it has no
way of knowing that.
What are your thoughts?
Tatsuhiko Miyagawa
> 1) Many web servers (e.g. nginx, lighttpd, older versions of IIS) do not
> accept POST requests with chunked body at all, they reject them with "411
> Length Required" and then they close the connection. This is not directly
> related to our situation but I thought it was interesting. BTW, you can
> download the script I created for this experiment from here:
> http://gist.github.com/559889.
Thanks for the investigations.
> So what should we do? From a theoretical point of view, a PSGI has 4
> possibilities regarding chunked bodies:
> a) Reject such requests with "411 Length Required" as soon as all request
> headers have been received. Then the connection would be closed (specifying
> "Connection: close" in the response headers) without doing any parsing of
> the body. That's exactly like case 1 above.
> b) Decode the body and remove the "Transfer-Encoding" header.
> c) Decode the body and keep the "Transfer-Encoding" header (just like
> apache/mod_cgi).
> d) Leave the "Transfer-Encoding" header and the body intact and let the PSGI
> app to do all the work.
Don't forget that PSGI is a specification (interface) *between* the
web servers and the applications, so a) sounds like it's out of the
border - defining what web servers should do should be out of scope.
"Web servers MAY reject such requests with 411 ... " is okay to say,
though.
> I believe that options c & d don't make much sense and should be disallowed
> by the PSGI spec.
To me they both look okay, as long as the application can know what's
going on and can do the right thing against the input stream. b) is
what Starman does right now, and is transparent to the application, so
it should be allowed. Again, choosing to do b) or c/d) should be
completely up to the server, not defined which to do in the
specification. Do we agree on that?
> Option c is clearly inelegant. Regarding option d: if a
> HTTP/1.1 server supports keep-alive connections then it must be able to
> recognize chunked bodies in order to be able to process subsequent pipelined
> requests.
I disagree (if I understand what you mean). Servers should take care
of recognizing chunked body boundaries and send EOF to the input
stream, and invoke the apps multiple times as long as keep-alive is in
use. Again, decoding the stream (like Starman does) or streaming is up
to the servers.
What I haven't determined yet is how to "tell" the application whether
the input stream is decoded or not.
> B) The PSGI spec should clearly indicate that psgi.input cannot be used to
> read past the end of the body.
>
>
> C) We must fix HTTP::Body. Unfortunately this cannot be done without an api
> change. Its constructor should accept an extra argument used to indicated
> chunked encoding. Additionally, there must be some mechanism to inform the
> HTTP::Body object that we have reached the end of the body because in the
> case of decoded bodies with unspecified length it has no way of knowing
> that.
Yes, once we define how to tell if psgi.input is decoded or not,
fixing HTTP::Body is easy.
>> On Mon, Aug 30, 2010 at 09:52:56PM -0700, Tatsuhiko Miyagawa wrote:
>>
>> Thanks.
>>
>> So the problem i found so far is the compatibility with HTTP::Body,
>> which is used by frameworks as well as Plack::Request. If given undef
>> $content_length HTTP::Body thinks the input stream is chunked, which
>> would cause a conflict because in our case the server (Starman)
>> dechunks for the app.
>>
>> This behavior is compatible to what mod_cgi does (STDIN is dechunked
>> if Transfer-Encoding is chunked), so I believe it's a good change in
>> the long term, but:
>>
>> a) existing apps that rely on HTTP::Body's auto dechunk would break -
>> we need to fix the code, or HTTP::Body to skip the dechunk code
>> (without mangling the 'chunked' attribute that has no setter right
>> now)
>>
>> b) Even if we change the app to assume the input stream is dechunked,
>> then that app won't work with other web servers that would NOT
>> dechunk. We might need an indicator in the $env (like
>> psgix.input.dechunked?) so that to see read($env->{'psgi.input'})
>> would return the raw chunked stream or dechunked stream.
>>
>> What do you think?
>
--
Tatsuhiko Miyagawa
Tatsuhiko Miyagawa
investigating what's the situation on this with Rack/WSGI servers,
such as Unicorn, Twiggy, gunicorn, Google App Engine, mod_wsgi etc.
that would be valuable.
--
Tatsuhiko Miyagawa
red...@gmx.com
>> I believe that options c & d don't make much sense and should be
>> disallowed
>> by the PSGI spec.
>
>To me they both look okay, as long as the application can know what's
>going on and can do the right thing against the input stream. b) is
>what Starman does right now, and is transparent to the application, so
>it should be allowed. Again, choosing to do b) or c/d) should be
>completely up to the server, not defined which to do in the
>specification. Do we agree on that?
OK then, let's do the following: It should be up to the server (web
server, PSGI implementation or PSGI middleware) to decide whether to
decode the message or not, but if the server decides to decode the
message then it should update the relevant headers. There only 2 headers
that are related to that, "Transfer-Encoding" and "Content-Encoding". So
for example, if a request contains the following headers:
Transfer-Encoding: chunked
Content-Encoding: gzip
then we should allow the following possibilities:
1) No decoding takes place, headers are left intact
2) The message is dechunked but not decompressed and the
"Transfer-Encoding" header is removed
3) The message is dechunked and decompressed and both headers are
removed
Another example: if a request contains the following header:
Transfer-Encoding: deflate, chunked
then the server could for example only dechunk the message without
inflating it. In that case it should change the header to
Transfer-Encoding: deflate
This corresponds to case 2 of the first example and of course there are
also options similar to 1 & 3.
The advantage of this approach is that we don't have to introduce any
new values in the PSGI environment, we just use two existing ones (i.e.
HTTP_TRANSFER_ENCODING and HTTP_CONTENT_ENCODING).
Additionally, if the server decides to decode all of the transfer
encodings and to buffer the entire request body, then it MAY also add a
"Content-Length" header (BTW only "Transfer-Encoding" conflicts with
"Content-Length", "Content-Encoding" with "Content-Length" is fine).
This is probably too obvious and we don't have to include that in the
specification.
Of course, as we have already discussed, the approach presented above is
incompatible to HTTP::Body in the cases where the server decides to do
some decoding (cases 2 & 3 above).
Mark Stosberg
I've been watching from the sidelines, but this proposal sounds good to
me. Thanks for proposing it!
Mark
--
. . . . . . . . . . . . . . . . . . . . . . . . . . .
Mark Stosberg Principal Developer
ma...@summersault.com Summersault, LLC
765-939-9301 ext 202 database driven websites
. . . . . http://www.summersault.com/ . . . . . . . .