You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ian Holsman <li...@holsman.net> on 2006/01/04 10:32:44 UTC
fcgi
I'm not sure why we aren't just reading the plen at the same time as the
clen... but as is when the 2nd header is read, it is not in sync (out by
padding-len bytes)
this patch makes it read at the same time, and it seems to make the
handler work for larger responses (as the following header is now synced
up properly)
--i
Index: mod_proxy_fcgi.c
===================================================================
--- mod_proxy_fcgi.c (revision 365863)
+++ mod_proxy_fcgi.c (working copy)
@@ -517,15 +517,15 @@
plen = fheader[6];
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
- "type %d request-id %d clen: %" APR_SIZE_T_FMT " plen %d",
+ "type %d request-id %d content len: %" APR_SIZE_T_FMT "
padding length %d",
type, rid,
clen, plen
);
recv_again:
- if (clen > sizeof(readbuf) - 1) {
+ if (clen +plen> sizeof(readbuf) - 1) {
readbuflen = sizeof(readbuf) - 1;
} else {
- readbuflen = clen;
+ readbuflen = clen+plen;
}
/* Now get the actual data. Yes it sucks to do this in a
second
@@ -582,11 +582,11 @@
/* If we didn't read all the data go back and get the
* rest of it. */
- if (clen > readbuflen) {
+ if (clen +plen > readbuflen) {
clen -= readbuflen;
goto recv_again;
}
-
+/*
if (plen) {
readbuflen = plen;
@@ -595,6 +595,7 @@
break;
}
}
+*/
} else {
b = apr_bucket_eos_create(c->bucket_alloc);
@@ -611,6 +612,9 @@
case FCGI_STDERR:
/* XXX TODO FCGI_STDERR gets written to the log file. */
+ ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server, "%*s",
+ readbuflen,
+ readbuf );
break;
case FCGI_END_REQUEST:
Re: fcgi
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 1/4/06, Jim Jagielski <ji...@jagunet.com> wrote:
> How are you testing? Let me see if I can recreate
> your test env here (or Paul's) so I can dig deeper.
I tested those changes with a simple fastcgi app that just dumps 50 or
60 bytes of content, then I manually messed with the size of the
buffers so that I could control how many times we'd have to read.
Paul tested with the Django tutorial application I believe, although
you'd have to ask him for more details on exactly how he was doing it.
-garrett
Re: fcgi
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jan 4, 2006, at 10:56 AM, Garrett Rooney wrote:
> On 1/4/06, Jim Jagielski <ji...@jagunet.com> wrote:
>>
>> On Jan 4, 2006, at 10:38 AM, Garrett Rooney wrote:
>>> See the list archives from last week for my
>>> attempts if you want someplace to start.
>>>
>>
>> You mean: Message-Id:
>> <7e...@mail.gmail.com> ??
>
> That exact message is the version we finally went with because it
> worked for both my tests and Paul's attempts to use it for the Django
> tutorial app. The previous versions in that thread tried to get both
> the padding and the content in one recv call, but never worked 100% of
> the time.
>
How are you testing? Let me see if I can recreate
your test env here (or Paul's) so I can dig deeper.
Re: fcgi
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 1/4/06, Jim Jagielski <ji...@jagunet.com> wrote:
>
> On Jan 4, 2006, at 10:38 AM, Garrett Rooney wrote:
> > See the list archives from last week for my
> > attempts if you want someplace to start.
> >
>
> You mean: Message-Id:
> <7e...@mail.gmail.com> ??
That exact message is the version we finally went with because it
worked for both my tests and Paul's attempts to use it for the Django
tutorial app. The previous versions in that thread tried to get both
the padding and the content in one recv call, but never worked 100% of
the time.
-garrett
Re: fcgi
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jan 4, 2006, at 10:38 AM, Garrett Rooney wrote:
> See the list archives from last week for my
> attempts if you want someplace to start.
>
You mean: Message-Id:
<7e...@mail.gmail.com> ??
Re: fcgi
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 1/4/06, Jim Jagielski <ji...@jagunet.com> wrote:
>
> On Jan 4, 2006, at 4:32 AM, Ian Holsman wrote:
>
> > I'm not sure why we aren't just reading the plen at the same time
> > as the clen... but as is when the 2nd header is read, it is not in
> > sync (out by padding-len bytes)
> >
> > this patch makes it read at the same time, and it seems to make the
> > handler work for larger responses (as the following header is now
> > synced up properly)
> >
>
> +1
The reason it isn't reading it all at once is that it's difficult to
get all the cases correct. This patch, for example, does not get it
right in all cases, if clen + plen > the amount read, but clen <
amount read, you aren't subtracting the remainder off of plen, so the
next read will get too much data and you'll be off again. I had a
version of this code in one of my patches that Paul didn't commit,
because while it tried really hard to get all the cases right it
screwed up some of them. See the list archives from last week for my
attempts if you want someplace to start.
I'd really like a better explenation of exactly what case we're
hitting here that's throwing off the parsing. I suppose it could be
that we're hitting a case where clen == 0 at the end, in which case
you can correct it by moving the cleanup plen read down to the bottom
of the FCGI_STDOUT case in the switch, instead of having it in the
clen != 0 part of the if:
Index: modules/proxy/mod_proxy_fcgi.c
===================================================================
--- modules/proxy/mod_proxy_fcgi.c (revision 365824)
+++ modules/proxy/mod_proxy_fcgi.c (working copy)
@@ -574,15 +574,6 @@
clen -= readbuflen;
goto recv_again;
}
-
- if (plen) {
- readbuflen = plen;
-
- rv = apr_socket_recv(conn->sock, readbuf,
&readbuflen);- if (rv != APR_SUCCESS) {
- break;
- }
- }
} else {
b = apr_bucket_eos_create(c->bucket_alloc);
@@ -595,6 +586,15 @@
/* XXX Why don't we cleanup here? (logic from AJP) */
}
+
+ if (plen) {
+ readbuflen = plen;
+
+ rv = apr_socket_recv(conn->sock, readbuf, &readbuflen);
+ if (rv != APR_SUCCESS) {
+ break;
+ }
+ }
break;
case FCGI_STDERR:
(totally untested, use at your own risk ;-)
-garrett
Re: fcgi
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jan 4, 2006, at 4:32 AM, Ian Holsman wrote:
> I'm not sure why we aren't just reading the plen at the same time
> as the clen... but as is when the 2nd header is read, it is not in
> sync (out by padding-len bytes)
>
> this patch makes it read at the same time, and it seems to make the
> handler work for larger responses (as the following header is now
> synced up properly)
>
+1
> +/*
> if (plen) {
> readbuflen = plen;
>
> @@ -595,6 +595,7 @@
> break;
> }
> }
> +*/
Could you, instead, #if 0 it out, for the time being?