You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by dg...@locus.apache.org on 2000/03/02 10:45:08 UTC
cvs commit: apache-2.0/src/main buff.c
dgaudet 00/03/02 01:45:08
Modified: src/main buff.c
Log:
fix chunked encoding.
this pretty much assumes/requires the API semantic clarification
i checked in previously to sendrecv.c -- which means we need to
actually fix all those XXXs before this works perfectly right.
specifically: bytes_written/read is always correct, regardless of
whether an error occured or not. this further propagates this type
of an API through buff.c -- gets rid of a few ifdefs manoj(?) left,
and fixes some other bugs.
plus i added a bunch of comments.
this passes a very simple CGI that pumps out a bunch of 4000 byte
packets. (used src/test/check_chunked to verify the output).
i haven't tried mod_test_chunk yet.
Revision Changes Path
1.32 +86 -52 apache-2.0/src/main/buff.c
Index: buff.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/main/buff.c,v
retrieving revision 1.31
retrieving revision 1.32
diff -u -r1.31 -r1.32
--- buff.c 2000/02/21 16:41:41 1.31
+++ buff.c 2000/03/02 09:45:07 1.32
@@ -220,12 +220,27 @@
return APR_EINVAL;
}
+/*
+ start chunked encoding.
+
+ in order for ap_bputc() to be an efficient macro we need to have
+ the buffer setup for the chunk already. if B_CHUNK is set, then
+ routines here using end_chunk() must be sure to call start_chunk()
+ or set an error condition before they return to the caller.
+ */
static void start_chunk(BUFF *fb)
{
fb->outchunk = fb->outcnt;
fb->outcnt += CHUNK_HEADER_SIZE;
}
+/*
+ we've got a chunk in progress which we want to end -- the caller
+ possibly wants to write extra bytes beyond those in fb->outbase.
+ if extra != 0 then the caller is assumed to write the chunk CRLF
+ trailer before calling start_chunk again. there's more info below
+ at large_write_chunk().
+*/
static int end_chunk(BUFF *fb, int extra)
{
int i;
@@ -247,7 +262,7 @@
chunk_size = MAX_CHUNK_SIZE;
}
- /* we know this will fit because of how we wrote it in start_chunk() */
+ /* we know this will fit because of space we saved in start_chunk() */
i = ap_snprintf((char *) &fb->outbase[fb->outchunk], CHUNK_HEADER_SIZE,
"%x", chunk_size);
@@ -264,9 +279,11 @@
*strp++ = '\015';
*strp = '\012';
- /* tack on the trailing CRLF, we've reserved room for this */
- fb->outbase[fb->outcnt++] = '\015';
- fb->outbase[fb->outcnt++] = '\012';
+ if (extra == 0) {
+ /* tack on the trailing CRLF, we've reserved room for this */
+ fb->outbase[fb->outcnt++] = '\015';
+ fb->outbase[fb->outcnt++] = '\012';
+ }
fb->outchunk = -1;
@@ -382,11 +399,7 @@
rv = read_with_errors(fb, buf, nbyte, &i);
if (rv != APR_SUCCESS) {
*bytes_read = nrd;
-#ifdef MIDWAY_ERROR_RETURNS_ERROR_BLAH_BLAH_BLAH
return rv;
-#else
- return *bytes_read ? APR_SUCCESS : rv;
-#endif
}
}
else {
@@ -395,11 +408,7 @@
rv = read_with_errors(fb, fb->inptr, fb->bufsiz, &i);
if (rv != APR_SUCCESS) {
*bytes_read = nrd;
-#ifdef MIDWAY_ERROR_RETURNS_ERROR_BLAH_BLAH_BLAH
return rv;
-#else
- return *bytes_read ? APR_SUCCESS : rv;
-#endif
}
fb->incnt = i;
if (i > nbyte)
@@ -556,13 +565,13 @@
{
ap_status_t rv;
rv = iol_writev(fb->iol, vec, nvec, bytes_written);
+ fb->bytes_sent += *bytes_written;
if (rv != APR_SUCCESS) {
fb->saved_errno = rv;
if (rv != APR_EAGAIN) {
doerror(fb, B_WR);
}
}
- fb->bytes_sent += *bytes_written;
return rv;
}
@@ -578,16 +587,10 @@
*bytes_written = 0;
while (i < nvec) {
rv = writev_with_errors(fb, vec + i, nvec - i, &n);
+ *bytes_written += n;
if (rv != APR_SUCCESS) {
-/* XXX - When we decide which way to go here, the ifdef will do away. the macro
- * is undefined by default */
-#ifdef MIDWAY_ERROR_RETURNS_ERROR_BLAH_BLAH_BLAH
return rv;
-#else
- return *bytes_written ? APR_SUCCESS : rv;
-#endif
}
- *bytes_written += n;
if (fb->flags & B_NONBLOCK) {
return APR_SUCCESS;
}
@@ -610,6 +613,9 @@
/* write the contents of fb->outbase, and buf,
stop at first partial write for a non-blocking buff
+
+ bytes_written is the number of bytes written from buf, it
+ doesn't include any bytes potentially written from fb->outbase.
*/
static ap_status_t large_write(BUFF *fb, const char *buf, ap_size_t nbyte,
ap_ssize_t *bytes_written)
@@ -632,22 +638,50 @@
nvec = 1;
}
rv = writev_it_all(fb, vec, nvec, bytes_written);
- if (*bytes_written == 0) { /* error or eof */
- return rv;
- }
if (*bytes_written < fb->outcnt) {
/* shift bytes forward in buffer */
fb->outcnt -= *bytes_written;
memmove(fb->outbase, fb->outbase + *bytes_written, fb->outcnt);
*bytes_written = 0;
- return APR_SUCCESS;
+ return rv;
}
*bytes_written -= fb->outcnt;
fb->outcnt = 0;
- return APR_SUCCESS;
+ return rv;
}
+/*
+ large_write_chunk is similar to large_write(), but used when chunking
+ is in effect.
+
+ in apache-1.3 we used bcwrite() and large_write() when writing large
+ chunks, and we did so in a way that caused us to write at least two
+ small buffers into the iovec -- one for the chunk size, and one for
+ the trailing CRLF. (we had iovecs of size 3 or 4 when chunking)
+
+ in 2.0 i wanted the ability to do non-blocking chunked i/o, which can
+ result in partial writes at times. this can be awfully inconvenient
+ with the 1.3 method of chunking because there are a godawful number
+ of special cases trying to remember how far into the iovec you got
+ before EAGAIN.
+
+ the insight i found was that we can combine the CRLF end of chunk
+ N with the size of chunk N+1. i do this right in the fb->outbase
+ buffer, because any partial write of this chunk end/start data is
+ the same as a partial write of any other buffered data. this gets
+ rid of almost all the special cases, and probably works out to be
+ faster than letting the kernel handle a 2 byte and a 6 byte buffer.
+
+ one special case is when we've told the client that we'd write a
+ chunk of K bytes, and we get an EAGAIN before we write K bytes.
+ i call this an "overcommit". in this case we keep note of the fact
+ that we've got an overcommit so that subsequent bwrites (which
+ the layer above BUFF is performing in the non-blocking case) can
+ decrement from the overcommit before they begin another chunk.
+
+ -djg
+*/
static ap_status_t large_write_chunk(BUFF *fb, const char *buf, ap_size_t nbyte,
ap_ssize_t *bytes_written)
{
@@ -655,40 +689,41 @@
ap_ssize_t total, n, amt;
ap_assert(nbyte > 0);
+ total = 0;
if (fb->chunk_overcommit) {
amt = nbyte > fb->chunk_overcommit ? fb->chunk_overcommit : nbyte;
rv = large_write(fb, buf, amt, &total);
- if (total == 0) { /* error or eof */
- *bytes_written = total;
- return rv;
- }
fb->chunk_overcommit -= total;
- if (fb->chunk_overcommit == 0) {
- fb->outbase[0] = '\015';
- fb->outbase[1] = '\012';
- fb->outcnt = 2;
- start_chunk(fb);
+ if (rv != APR_SUCCESS) {
+ *bytes_written = total;
+ return rv;
}
- if (total < amt || amt == nbyte) {
- *bytes_written = total;
+ if (fb->chunk_overcommit) {
+ /* above we ensured that we'd write at most the overcommit
+ bytes -- so either we had a partial write,
+ or we used up all of buf. either way we're done. */
+ ap_assert(amt < fb->chunk_overcommit || *bytes_written < amt);
+ *bytes_written = total;
return APR_SUCCESS;
}
+ /* we've finished the overcommit so we end off the previous
+ chunk, begin a new one, and shift buf */
+ fb->outbase[0] = '\015';
+ fb->outbase[1] = '\012';
+ fb->outcnt = 2;
+ start_chunk(fb);
nbyte -= total;
buf += total;
}
ap_assert(fb->chunk_overcommit == 0 && fb->outchunk != -1);
- total = 0;
- do {
+ while (nbyte) {
amt = end_chunk(fb, nbyte);
rv = large_write(fb, buf, amt, &n);
if (n < amt) {
+ /* can only be non-blocking or an error... */
fb->chunk_overcommit = amt - n;
*bytes_written = total + n;
-#ifdef MIDWAY_ERROR_RETURNS_ERROR_BLAH_BLAH_BLAH
return rv;
-#else
- return *bytes_written ? APR_SUCCESS : rv;
-#endif
}
fb->outbase[0] = '\015';
fb->outbase[1] = '\012';
@@ -697,7 +732,7 @@
nbyte -= amt;
buf += amt;
total += amt;
- } while (nbyte);
+ }
*bytes_written = total;
return APR_SUCCESS;
}
@@ -711,13 +746,13 @@
ap_status_t rv;
rv = iol_write(fb->iol, buf, nbyte, bytes_written);
+ fb->bytes_sent += *bytes_written;
if (rv != APR_SUCCESS) {
fb->saved_errno = rv;
if (rv != APR_EAGAIN) {
doerror(fb, B_WR);
}
}
- fb->bytes_sent += *bytes_written;
return rv;
}
@@ -734,14 +769,9 @@
while (fb->outcnt > 0) {
rv = write_with_errors(fb, fb->outbase + *bytes_written, fb->outcnt,
&n);
- if (n <= 0) { /* error or eof */
+ if (rv != APR_SUCCESS) {
if (*bytes_written) {
memmove(fb->outbase, fb->outbase + *bytes_written, fb->outcnt);
-#ifdef MIDWAY_ERROR_RETURNS_ERROR_BLAH_BLAH_BLAH
- return rv;
-#else
- return APR_SUCCESS;
-#endif
}
return rv;
}
@@ -768,6 +798,7 @@
int amt;
int total;
ap_ssize_t n;
+ ap_status_t rv;
if (fb->flags & (B_WRERR | B_EOUT)) {
*bytes_written = 0;
@@ -803,8 +834,11 @@
fb->outcnt += amt;
buf = (const char *) buf + amt;
nbyte -= amt;
- (void) bflush_core(fb, &n);
- if (n < amt) {
+ rv = bflush_core(fb, &n);
+ if (rv != APR_SUCCESS) {
+ /* don't continue past an error or a partial write */
+ /* regardless of how much flushed we've at least copied amt
+ into our buffers... */
*bytes_written = amt;
return APR_SUCCESS;
}
Re: cvs commit: apache-2.0/src/main buff.c
Posted by Manoj Kasichainula <ma...@io.com>.
[Today, I've caught up with list mail from December...]
On Sun, Mar 12, 2000 at 09:41:22AM -0800, Dean Gaudet wrote:
> On Tue, 7 Mar 2000, Manoj Kasichainula wrote:
>
> > On Thu, Mar 02, 2000 at 02:48:47PM -0800, Dean Gaudet wrote:
> > > On Thu, 2 Mar 2000, Manoj Kasichainula wrote:
> > > > Hmmm. So now, the buff calls return errors even when bytes have been
> > > > successfully read or written. This wasn't what you wanted to do
> > > > before.
> > >
> > > bwrite and bread do, sure. but i didn't propagate that into the rest of
> > > the buffered operations... dunno really, it was the only thing which made
> > > sense -- since i needed the info the whole way up the call chain.
OK, the quick way to ask my question: Why is the error status needed
before ap_b{read,write} gets called again?
> > OK, since I'm too lazy to figure this out...
>
> i didn't touch ap_bread, ap_bwrite prototypes, so i assume you've already
> propagated the necessary ap_status_t changes to the rest of the code...
Yup.
> and to be honest, i'm too lazy to go look at the code again. if it
> bothers you, then you do it. :)
meaniehead.
> > Why is that error status needed immediately? With the old POSIX-like
> > behavior, the call chain would get APR_SUCCESS and the number of bytes
> > successfully written. It would process all that, be happy, then try to
> > read more and get the error. With this change, now the error is
> > propogated up immediately.
>
> please distinguish between "propagated up through internal functions
> within buff.c" and "propagated up to the buff.c callers" 'cause your
> language is too confusing to parse.
The latter. Callers of ap_bread and ap_bwrite to be specific.
> > This is a good thing, but we were worried at one point that the rest
> > of the Apache code doesn't know how to deal with both bytes and an
> > error code.
>
> like i said, i didn't touch the ap_bread/bwrite prototypes... so i'm not
> sure what you're asking about.
The prototypes support both modes of operation. The #defines you
removed only selected whether the added flexibility of error+bytes at
the same time was actually used. Now it is, which I think is a good
thing, but 3rd-party modules (and maybe some Apache code) will have to
be prepared for this.
Re: cvs commit: apache-2.0/src/main buff.c
Posted by Dean Gaudet <dg...@arctic.org>.
On Tue, 7 Mar 2000, Manoj Kasichainula wrote:
> On Thu, Mar 02, 2000 at 02:48:47PM -0800, Dean Gaudet wrote:
> > On Thu, 2 Mar 2000, Manoj Kasichainula wrote:
> > > Hmmm. So now, the buff calls return errors even when bytes have been
> > > successfully read or written. This wasn't what you wanted to do
> > > before.
> >
> > bwrite and bread do, sure. but i didn't propagate that into the rest of
> > the buffered operations... dunno really, it was the only thing which made
> > sense -- since i needed the info the whole way up the call chain.
>
> OK, since I'm too lazy to figure this out...
i didn't touch ap_bread, ap_bwrite prototypes, so i assume you've already
propagated the necessary ap_status_t changes to the rest of the code...
and so i'm not really sure at all what you're confused about.
and to be honest, i'm too lazy to go look at the code again. if it
bothers you, then you do it. :)
> Why is that error status needed immediately? With the old POSIX-like
> behavior, the call chain would get APR_SUCCESS and the number of bytes
> successfully written. It would process all that, be happy, then try to
> read more and get the error. With this change, now the error is
> propogated up immediately.
please distinguish between "propagated up through internal functions
within buff.c" and "propagated up to the buff.c callers" 'cause your
language is too confusing to parse.
> This is a good thing, but we were worried at one point that the rest
> of the Apache code doesn't know how to deal with both bytes and an
> error code.
like i said, i didn't touch the ap_bread/bwrite prototypes... so i'm not
sure what you're asking about.
Dean
Re: cvs commit: apache-2.0/src/main buff.c
Posted by Manoj Kasichainula <ma...@io.com>.
On Thu, Mar 02, 2000 at 02:48:47PM -0800, Dean Gaudet wrote:
> On Thu, 2 Mar 2000, Manoj Kasichainula wrote:
> > Hmmm. So now, the buff calls return errors even when bytes have been
> > successfully read or written. This wasn't what you wanted to do
> > before.
>
> bwrite and bread do, sure. but i didn't propagate that into the rest of
> the buffered operations... dunno really, it was the only thing which made
> sense -- since i needed the info the whole way up the call chain.
OK, since I'm too lazy to figure this out...
Why is that error status needed immediately? With the old POSIX-like
behavior, the call chain would get APR_SUCCESS and the number of bytes
successfully written. It would process all that, be happy, then try to
read more and get the error. With this change, now the error is
propogated up immediately.
This is a good thing, but we were worried at one point that the rest
of the Apache code doesn't know how to deal with both bytes and an
error code.
Re: cvs commit: apache-2.0/src/main buff.c
Posted by Dean Gaudet <dg...@arctic.org>.
On Thu, 2 Mar 2000, Manoj Kasichainula wrote:
> On Thu, Mar 02, 2000 at 09:45:08AM -0000, dgaudet@locus.apache.org wrote:
> > @@ -382,11 +399,7 @@
> > rv = read_with_errors(fb, buf, nbyte, &i);
> > if (rv != APR_SUCCESS) {
> > *bytes_read = nrd;
> > -#ifdef MIDWAY_ERROR_RETURNS_ERROR_BLAH_BLAH_BLAH
> > return rv;
> > -#else
> > - return *bytes_read ? APR_SUCCESS : rv;
> > -#endif
>
> Hmmm. So now, the buff calls return errors even when bytes have been
> successfully read or written. This wasn't what you wanted to do
> before.
bwrite and bread do, sure. but i didn't propagate that into the rest of
the buffered operations... dunno really, it was the only thing which made
sense -- since i needed the info the whole way up the call chain.
Dean
Re: cvs commit: apache-2.0/src/main buff.c
Posted by Manoj Kasichainula <ma...@io.com>.
On Thu, Mar 02, 2000 at 09:45:08AM -0000, dgaudet@locus.apache.org wrote:
> @@ -382,11 +399,7 @@
> rv = read_with_errors(fb, buf, nbyte, &i);
> if (rv != APR_SUCCESS) {
> *bytes_read = nrd;
> -#ifdef MIDWAY_ERROR_RETURNS_ERROR_BLAH_BLAH_BLAH
> return rv;
> -#else
> - return *bytes_read ? APR_SUCCESS : rv;
> -#endif
Hmmm. So now, the buff calls return errors even when bytes have been
successfully read or written. This wasn't what you wanted to do
before.