You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Dean Gaudet <dg...@arctic.org> on 1997/01/28 21:09:11 UTC
[PATCH] take 2: improved flushing behaviour
This supercedes the patch I posted last night, so patch -R that one if you've
already applied it.
btestread() has been eliminated in favour of B_SAFEREAD. Essentially we
are delaying the flush decision until the BUFF code is actually about to
read the next request. The saferead() function in buff.c takes care of
some of the details that we previously took care of at each read().
I've given this a bit of a workout.
I've discovered what looks like a bug elsewhere -- when we send
back a 304 we also send a Connection: close (presumably because
there's no Content-Length). So when netscape is verifying a page like
<http://www.arctic.org/~dgaudet/all-icons> it has to open a connection
for each GET If-Modified-Since. Protocol cops?
Dean
Index: buff.c
===================================================================
RCS file: /export/home/cvs/apache/src/buff.c,v
retrieving revision 1.15
diff -c -3 -r1.15 buff.c
*** buff.c 1997/01/25 22:37:10 1.15
--- buff.c 1997/01/28 19:58:37
***************
*** 175,193 ****
}
/*
! * Set a flag on (1) or off (0). Currently, these flags work
! * as a function of the bcwrite() function, so we make sure to
! * flush before setting them one way or the other; otherwise
! * writes could end up with the wrong flag.
*/
int bsetflag(BUFF *fb, int flag, int value)
{
! bflush(fb);
if (value) fb->flags |= flag;
else fb->flags &= ~flag;
return value;
}
/*
* Read up to nbyte bytes into buf.
* If fewer than byte bytes are currently available, then return those.
--- 175,239 ----
}
/*
! * Set a flag on (1) or off (0).
*/
int bsetflag(BUFF *fb, int flag, int value)
{
! if( flag & B_CHUNK ) {
! /*
! * This shouldn't be true for much longer -djg
! * Currently, these flags work
! * as a function of the bcwrite() function, so we make sure to
! * flush before setting them one way or the other; otherwise
! * writes could end up with the wrong flag.
! */
! bflush(fb);
! }
if (value) fb->flags |= flag;
else fb->flags &= ~flag;
return value;
}
+
+ /*
+ * This is called instead of read() everywhere in here. It implements
+ * the B_SAFEREAD functionality -- which is to force a flush() if a read()
+ * would block. It also deals with the EINTR errno result from read().
+ * return code is like read() except EINTR is eliminated.
+ */
+ static int
+ saferead( BUFF *fb, void *buf, int nbyte )
+ {
+ int rv;
+
+ if( fb->flags & B_SAFEREAD ) {
+ fd_set fds;
+ struct timeval tv;
+
+ /* test for a block */
+ do {
+ FD_ZERO( &fds );
+ FD_SET( fb->fd_in, &fds );
+ tv.tv_sec = 0;
+ tv.tv_usec = 0;
+ #ifdef HPUX
+ rv = select( fb->fd_in + 1, (int *)&fds, NULL, NULL, &tv );
+ #else
+ rv = select( fb->fd_in + 1, &fds, NULL, NULL, &tv );
+ #endif
+ } while( rv < 0 && errno == EINTR );
+ /* treat any error as if it would block as well */
+ if( rv != 1 ) {
+ bflush(fb);
+ }
+ }
+ do {
+ rv = read( fb->fd_in, buf, nbyte );
+ } while ( rv == -1 && errno == EINTR );
+ return( rv );
+ }
+
+
/*
* Read up to nbyte bytes into buf.
* If fewer than byte bytes are currently available, then return those.
***************
*** 204,211 ****
if (!(fb->flags & B_RD))
{
/* Unbuffered reading */
! do i = read(fb->fd_in, buf, nbyte);
! while (i == -1 && errno == EINTR);
if (i == -1 && errno != EAGAIN) doerror(fb, B_RD);
return i;
}
--- 250,256 ----
if (!(fb->flags & B_RD))
{
/* Unbuffered reading */
! i = saferead( fb, buf, nbyte );
if (i == -1 && errno != EAGAIN) doerror(fb, B_RD);
return i;
}
***************
*** 233,240 ****
if (nbyte >= fb->bufsiz)
{
/* read directly into buffer */
! do i = read(fb->fd_in, buf, nbyte);
! while (i == -1 && errno == EINTR);
if (i == -1)
{
if (nrd == 0)
--- 278,284 ----
if (nbyte >= fb->bufsiz)
{
/* read directly into buffer */
! i = saferead( fb, buf, nbyte );
if (i == -1)
{
if (nrd == 0)
***************
*** 248,255 ****
{
/* read into hold buffer, then memcpy */
fb->inptr = fb->inbase;
! do i = read(fb->fd_in, fb->inptr, fb->bufsiz);
! while (i == -1 && errno == EINTR);
if (i == -1)
{
if (nrd == 0)
--- 292,298 ----
{
/* read into hold buffer, then memcpy */
fb->inptr = fb->inbase;
! i = saferead( fb, fb->inptr, fb->bufsiz );
if (i == -1)
{
if (nrd == 0)
***************
*** 310,317 ****
fb->inptr = fb->inbase;
fb->incnt = 0;
if (fb->flags & B_EOF) break;
! do i = read(fb->fd_in, fb->inptr, fb->bufsiz);
! while (i == -1 && errno == EINTR);
if (i == -1)
{
buff[ct] = '\0';
--- 353,359 ----
fb->inptr = fb->inbase;
fb->incnt = 0;
if (fb->flags & B_EOF) break;
! i = saferead( fb, fb->inptr, fb->bufsiz );
if (i == -1)
{
buff[ct] = '\0';
***************
*** 381,389 ****
if (fb->flags & B_EOF)
return 0;
! do {
! i = read(fb->fd_in, fb->inptr, fb->bufsiz);
! } while (i == -1 && errno == EINTR);
if (i == -1) {
if (errno != EAGAIN)
--- 423,429 ----
if (fb->flags & B_EOF)
return 0;
! i = saferead( fb, fb->inptr, fb->bufsiz );
if (i == -1) {
if (errno != EAGAIN)
***************
*** 402,438 ****
}
/*
- * Tests if there is data to be read. Returns 0 if there is data,
- * -1 otherwise.
- */
- int
- btestread(BUFF *fb)
- {
- fd_set fds;
- struct timeval tv;
- int rv;
-
- /* the simple case, we've already got data in the buffer */
- if( fb->incnt ) {
- return( 0 );
- }
-
- /* otherwise see if the descriptor would block if we try to read it */
- do {
- FD_ZERO( &fds );
- FD_SET( fb->fd_in, &fds );
- tv.tv_sec = 0;
- tv.tv_usec = 0;
- #ifdef HPUX
- rv = select( fb->fd_in + 1, (int *)&fds, NULL, NULL, &tv );
- #else
- rv = select( fb->fd_in + 1, &fds, NULL, NULL, &tv );
- #endif
- } while( rv < 0 && errno == EINTR );
- return( rv == 1 ? 0 : -1 );
- }
-
- /*
* Skip data until a linefeed character is read
* Returns 1 on success, 0 if no LF found, or -1 on error
*/
--- 442,447 ----
***************
*** 464,471 ****
fb->inptr = fb->inbase;
fb->incnt = 0;
if (fb->flags & B_EOF) return 0;
! do i = read(fb->fd_in, fb->inptr, fb->bufsiz);
! while (i == -1 && errno == EINTR);
if (i == 0) fb->flags |= B_EOF;
if (i == -1 && errno != EAGAIN) doerror(fb, B_RD);
if (i == 0 || i == -1) return i;
--- 473,479 ----
fb->inptr = fb->inbase;
fb->incnt = 0;
if (fb->flags & B_EOF) return 0;
! i = saferead( fb, fb->inptr, fb->bufsiz );
if (i == 0) fb->flags |= B_EOF;
if (i == -1 && errno != EAGAIN) doerror(fb, B_RD);
if (i == 0 || i == -1) return i;
Index: buff.h
===================================================================
RCS file: /export/home/cvs/apache/src/buff.h,v
retrieving revision 1.10
diff -c -3 -r1.10 buff.h
*** buff.h 1997/01/25 22:37:09 1.10
--- buff.h 1997/01/28 19:58:38
***************
*** 68,73 ****
--- 68,75 ----
#define B_ERROR (48)
/* Use chunked writing */
#define B_CHUNK (64)
+ /* bflush() if a read would block */
+ #define B_SAFEREAD (128)
typedef struct buff_struct BUFF;
***************
*** 120,126 ****
extern int bvputs(BUFF *fb, ...);
extern int bprintf(BUFF *fb,const char *fmt,...);
extern int vbprintf(BUFF *fb,const char *fmt,va_list vlist);
- extern int btestread(BUFF *fb);
/* Internal routines */
extern int bflsbuf(int c, BUFF *fb);
--- 122,127 ----
Index: http_main.c
===================================================================
RCS file: /export/home/cvs/apache/src/http_main.c,v
retrieving revision 1.114
diff -c -3 -r1.114 http_main.c
*** http_main.c 1997/01/26 21:11:53 1.114
--- http_main.c 1997/01/28 19:58:38
***************
*** 1675,1684 ****
if (r) increment_counts(child_num,r,1);
#endif
while (r && current_conn->keepalive) {
- /* If there's no request waiting for us to read then flush the
- * socket. This is to avoid tickling a bug in older Netscape
- * clients. */
- if( btestread(conn_io) == -1 ) bflush(conn_io);
destroy_pool(r->pool);
(void)update_child_status (child_num, SERVER_BUSY_KEEPALIVE,
(request_rec*)NULL);
--- 1675,1680 ----
***************
*** 2153,2159 ****
if (r) process_request (r); /* else premature EOF (ignore) */
while (r && conn->keepalive) {
- if( btestread(cio) == -1 ) bflush(cio);
destroy_pool(r->pool);
r = read_request (conn);
if (r) process_request (r);
--- 2149,2154 ----
Index: http_protocol.c
===================================================================
RCS file: /export/home/cvs/apache/src/http_protocol.c,v
retrieving revision 1.93
diff -c -3 -r1.93 http_protocol.c
*** http_protocol.c 1997/01/26 01:15:13 1.93
--- http_protocol.c 1997/01/28 19:58:38
***************
*** 520,530 ****
/* Read past empty lines until we get a real request line,
* a read error, the connection closes (EOF), or we timeout.
*/
while ((len = getline(l, HUGE_STRING_LEN, conn->client, 0)) <= 0) {
! if ((len < 0) || bgetflag(conn->client, B_EOF))
return 0;
}
if (len == (HUGE_STRING_LEN - 1))
return 0; /* Should be a 414 error status instead */
--- 520,545 ----
/* Read past empty lines until we get a real request line,
* a read error, the connection closes (EOF), or we timeout.
+ *
+ * We skip empty lines because browsers have to tack a CRLF on to the end
+ * of POSTs to support old CERN webservers. But note that we may not
+ * have flushed any previous response completely to the client yet.
+ * We delay the flush as long as possible so that we can improve
+ * performance for clients that are pipelining requests. If a request
+ * is pipelined then we won't block during the (implicit) read() below.
+ * If the requests aren't pipelined, then the client is still waiting
+ * for the final buffer flush from us, and we will block in the implicit
+ * read(). B_SAFEREAD ensures that the BUFF layer flushes if it will
+ * have to block during a read.
*/
+ bsetflag( conn->client, B_SAFEREAD, 1 );
while ((len = getline(l, HUGE_STRING_LEN, conn->client, 0)) <= 0) {
! if ((len < 0) || bgetflag(conn->client, B_EOF)) {
! bsetflag( conn->client, B_SAFEREAD, 0 );
return 0;
+ }
}
+ bsetflag( conn->client, B_SAFEREAD, 0 );
if (len == (HUGE_STRING_LEN - 1))
return 0; /* Should be a 414 error status instead */
Re: 304 and keepalive bug
Posted by Chuck Murcko <ch...@topsail.org>.
Alexei Kosut wrote:
>
> On Tue, 28 Jan 1997, Dean Gaudet wrote:
>
> > I tried with the patch below included and navigator was able to do
> > keepalive properly on 304 responses. So this is definately an apache bug.
> > I'm not sure of the correct solution, that set_keepalive if() statement is
> > a monster mess.
>
> It's not a bug - Apache's not required to support persistent
> connections at all. On the other hand, it's a nice feature, and I can
> see no reason why Apache shouldn't keep persistent connections alive.
>
> +1 on your patch.
>
+1 also. A separate mechanism would be needed to deal with the bodied
messages. The fact that error messages in Apache can be customized makes
that a real headache.
--
chuck
Chuck Murcko
The Topsail Group, West Chester PA USA
chuck@topsail.org
Re: 304 and keepalive bug
Posted by Dean Gaudet <dg...@arctic.org>.
Ah ok I feel better about my patch then.
Dean
On Wed, 29 Jan 1997, Alexei Kosut wrote:
> On Wed, 29 Jan 1997, Dean Gaudet wrote:
>
> > But is my patch really the best way to do it? Shouldn't all 300, 400, and
> > 500 responses allow keepalive? It's only 200s that don't have
> > content-length (and can't chunk encode) that I can think of which should
> > send Connection: close. But I've never looked into this part of the
> > server very much.
>
> All error responses *should*. The problem has nothing to do with that,
> but with the way Apache creates the message bodies: there's no way to
> determine a content-length for these responses without rewriting all
> that code. No one ever bothered to do so. 304 doesn't have this
> problem because it's bodyless.
>
> And, of course, some other changes would have to be made; for example,
> if I POST to a non-POSTable file, the server will return an error, but
> the POST data is still sitting there in the input stream, and if we
> maintained a persistent connection here, that'd be a problem. And, of
> course, you run into other problems here: mod_cgi, for example, can
> return an error both before and after it reads POST data. Currently,
> the server has no way of knowing this. And what if the error is
> returned in the middle of reading in the data? You'd need some more
> functionality in the *_client_block functions, and then tap into that
> when creating the error.
>
> In other words, there's no fundamental reason why we shouldn't
> establish persistent connections for all responses, there are just
> some things that need to be done that no one has.
>
> --
> ________________________________________________________________________
> Alexei Kosut <ak...@nueva.pvt.k12.ca.us> The Apache HTTP Server
> URL: http://www.nueva.pvt.k12.ca.us/~akosut/ http://www.apache.org/
>
>
Re: 304 and keepalive bug
Posted by Alexei Kosut <ak...@nueva.pvt.k12.ca.us>.
On Wed, 29 Jan 1997, Dean Gaudet wrote:
> But is my patch really the best way to do it? Shouldn't all 300, 400, and
> 500 responses allow keepalive? It's only 200s that don't have
> content-length (and can't chunk encode) that I can think of which should
> send Connection: close. But I've never looked into this part of the
> server very much.
All error responses *should*. The problem has nothing to do with that,
but with the way Apache creates the message bodies: there's no way to
determine a content-length for these responses without rewriting all
that code. No one ever bothered to do so. 304 doesn't have this
problem because it's bodyless.
And, of course, some other changes would have to be made; for example,
if I POST to a non-POSTable file, the server will return an error, but
the POST data is still sitting there in the input stream, and if we
maintained a persistent connection here, that'd be a problem. And, of
course, you run into other problems here: mod_cgi, for example, can
return an error both before and after it reads POST data. Currently,
the server has no way of knowing this. And what if the error is
returned in the middle of reading in the data? You'd need some more
functionality in the *_client_block functions, and then tap into that
when creating the error.
In other words, there's no fundamental reason why we shouldn't
establish persistent connections for all responses, there are just
some things that need to be done that no one has.
--
________________________________________________________________________
Alexei Kosut <ak...@nueva.pvt.k12.ca.us> The Apache HTTP Server
URL: http://www.nueva.pvt.k12.ca.us/~akosut/ http://www.apache.org/
Re: 304 and keepalive bug
Posted by Dean Gaudet <dg...@arctic.org>.
But is my patch really the best way to do it? Shouldn't all 300, 400, and
500 responses allow keepalive? It's only 200s that don't have
content-length (and can't chunk encode) that I can think of which should
send Connection: close. But I've never looked into this part of the
server very much.
Dean
On Tue, 28 Jan 1997, Alexei Kosut wrote:
> On Tue, 28 Jan 1997, Dean Gaudet wrote:
>
> > I tried with the patch below included and navigator was able to do
> > keepalive properly on 304 responses. So this is definately an apache bug.
> > I'm not sure of the correct solution, that set_keepalive if() statement is
> > a monster mess.
>
> It's not a bug - Apache's not required to support persistent
> connections at all. On the other hand, it's a nice feature, and I can
> see no reason why Apache shouldn't keep persistent connections alive.
>
> +1 on your patch.
>
> --
> ________________________________________________________________________
> Alexei Kosut <ak...@nueva.pvt.k12.ca.us> The Apache HTTP Server
> URL: http://www.nueva.pvt.k12.ca.us/~akosut/ http://www.apache.org/
>
>
Re: 304 and keepalive bug
Posted by Alexei Kosut <ak...@nueva.pvt.k12.ca.us>.
On Tue, 28 Jan 1997, Dean Gaudet wrote:
> I tried with the patch below included and navigator was able to do
> keepalive properly on 304 responses. So this is definately an apache bug.
> I'm not sure of the correct solution, that set_keepalive if() statement is
> a monster mess.
It's not a bug - Apache's not required to support persistent
connections at all. On the other hand, it's a nice feature, and I can
see no reason why Apache shouldn't keep persistent connections alive.
+1 on your patch.
--
________________________________________________________________________
Alexei Kosut <ak...@nueva.pvt.k12.ca.us> The Apache HTTP Server
URL: http://www.nueva.pvt.k12.ca.us/~akosut/ http://www.apache.org/
304 and keepalive bug
Posted by Dean Gaudet <dg...@arctic.org>.
On Tue, 28 Jan 1997, Dean Gaudet wrote:
> I've discovered what looks like a bug elsewhere -- when we send
> back a 304 we also send a Connection: close (presumably because
> there's no Content-Length). So when netscape is verifying a page like
> <http://www.arctic.org/~dgaudet/all-icons> it has to open a connection
> for each GET If-Modified-Since. Protocol cops?
I tried with the patch below included and navigator was able to do
keepalive properly on 304 responses. So this is definately an apache bug.
I'm not sure of the correct solution, that set_keepalive if() statement is
a monster mess.
Incidentally, the difference on the wire is a difference of 744 packets
versus 211.
Dean
Index: http_protocol.c
===================================================================
RCS file: /export/home/cvs/apache/src/http_protocol.c,v
retrieving revision 1.93
diff -c -3 -r1.93 http_protocol.c
*** http_protocol.c 1997/01/26 01:15:13 1.93
--- http_protocol.c 1997/01/28 20:26:02
***************
*** 216,222 ****
else if (r->server->keep_alive && (!r->server->keep_alive_max ||
(r->server->keep_alive_max > r->connection->keepalives)) &&
(r->server->keep_alive_timeout > 0) &&
! (r->header_only || length || tenc ||
((r->proto_num >= 1001) && (r->byterange > 1 || (r->chunked = 1)))) &&
(!find_token(r->pool, conn, "close")) &&
((ka_sent = find_token(r->pool, conn, "keep-alive")) ||
--- 216,222 ----
else if (r->server->keep_alive && (!r->server->keep_alive_max ||
(r->server->keep_alive_max > r->connection->keepalives)) &&
(r->server->keep_alive_timeout > 0) &&
! (r->status == USE_LOCAL_COPY || r->header_only || length || tenc ||
((r->proto_num >= 1001) && (r->byterange > 1 || (r->chunked = 1)))) &&
(!find_token(r->pool, conn, "close")) &&
((ka_sent = find_token(r->pool, conn, "keep-alive")) ||