You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Filip Hanik - Dev Lists <de...@hanik.com> on 2007/03/02 22:05:57 UTC
Re: Small patch to ab apr_socket_recv error handling
is the patch below looking good?
does it need adjustments?
do I need to follow a different process?
Filip
Filip Hanik - Dev Lists wrote:
> ok, final patch, this one also adds in Content-Length: 0 when keep
> alive is used.
> somehow, most containers will not do keep alive unless there is a
> content length header.
>
> Filip
>
> Filip Hanik - Dev Lists wrote:
>> hi Aaron,
>> I added in the "-r" command line options, to not exit out on
>> apr_socket_recv errors.
>> Patch attached
>>
>> Filip
>>
>
> ------------------------------------------------------------------------
>
> Index: ab.c
> ===================================================================
> --- ab.c (revision 511976)
> +++ ab.c (working copy)
> @@ -258,6 +258,7 @@
> /* --------------------- GLOBALS ---------------------------- */
>
> int verbosity = 0; /* no verbosity by default */
> +int recverrok = 0;
> int posting = 0; /* GET by default */
> int requests = 1; /* Number of requests to make */
> int heartbeatres = 100; /* How often do we say we're alive */
> @@ -1330,9 +1331,19 @@
> /* catch legitimate fatal apr_socket_recv errors */
> else if (status != APR_SUCCESS) {
> err_except++; /* XXX: is this the right error counter? */
> - /* XXX: Should errors here be fatal, or should we allow a
> - * certain number of them before completely failing? -aaron */
> - apr_err("apr_socket_recv", status);
> + if ( recverrok ) {
> + bad++;
> + close_connection(c);
> + if ( verbosity >= 1 ) {
> + char buf[120];
> + fprintf(stderr,"%s: %s (%d)\n","apr_socket_recv", apr_strerror(status, buf, sizeof buf), status);
> + }
> + return;
> + } else {
> + /* XXX: Should errors here be fatal, or should we allow a
> + * certain number of them before completely failing? -aaron */
> + apr_err("apr_socket_recv", status);
> + }
> }
> }
>
> @@ -1559,7 +1570,7 @@
> (posting == 0) ? "GET" : "HEAD",
> (isproxy) ? fullurl : path,
> AP_AB_BASEREVISION,
> - keepalive ? "Connection: Keep-Alive\r\n" : "",
> + keepalive ? "Connection: Keep-Alive\r\nContent-Length: 0\r\n" : "",
> cookie, auth, host_field, colonhost, hdrs);
> }
> else {
> @@ -1819,6 +1830,7 @@
> fprintf(stderr, " -S Do not show confidence estimators and warnings.\n");
> fprintf(stderr, " -g filename Output collected data to gnuplot format file.\n");
> fprintf(stderr, " -e filename Output CSV file with percentages served\n");
> + fprintf(stderr, " -r Don't exit on apr_socket_recv errors.\n");
> fprintf(stderr, " -h Display usage information (this message)\n");
> #ifdef USE_SSL
> fprintf(stderr, " -Z ciphersuite Specify SSL/TLS cipher suite (See openssl ciphers)\n");
> @@ -1981,7 +1993,7 @@
> #endif
>
> apr_getopt_init(&opt, cntxt, argc, argv);
> - while ((status = apr_getopt(opt, "n:c:t:b:T:p:v:kVhwix:y:z:C:H:P:A:g:X:de:Sq"
> + while ((status = apr_getopt(opt, "n:c:t:b:T:p:v:rkVhwix:y:z:C:H:P:A:g:X:de:Sq"
> #ifdef USE_SSL
> "Z:f:"
> #endif
> @@ -2032,6 +2044,9 @@
> exit(r);
> }
> break;
> + case 'r':
> + recverrok = 1;
> + break;
> case 'v':
> verbosity = atoi(optarg);
> break;
>
> ------------------------------------------------------------------------
>
> No virus found in this incoming message.
> Checked by AVG Free Edition.
> Version: 7.5.446 / Virus Database: 268.18.4/705 - Release Date: 2/27/2007 3:24 PM
>
Re: Small patch to ab apr_socket_recv error handling
Posted by Jeff Trawick <tr...@gmail.com>.
On 3/8/07, Filip Hanik - Dev Lists <de...@hanik.com> wrote:
> it is an attachment, chances are your mail reader is expanding it into
> your viewing window
> but you can also get it here
>
> http://www.hanik.com/fix-ab-recv-error.patch
thanks; committed to trunk
Re: Small patch to ab apr_socket_recv error handling
Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
it is an attachment, chances are your mail reader is expanding it into
your viewing window
but you can also get it here
http://www.hanik.com/fix-ab-recv-error.patch
Filip
Jeff Trawick wrote:
> On 3/7/07, Filip Hanik - Dev Lists <de...@hanik.com> wrote:
>> ok, Jeff's feedback has been incorporated into this patch.
>
> Could you post the patch again as an attachment? Some whitespace
> oddity is making it hard to apply for me.
>
> Thanks!
>
>
Re: Small patch to ab apr_socket_recv error handling
Posted by Jeff Trawick <tr...@gmail.com>.
On 3/7/07, Filip Hanik - Dev Lists <de...@hanik.com> wrote:
> ok, Jeff's feedback has been incorporated into this patch.
Could you post the patch again as an attachment? Some whitespace
oddity is making it hard to apply for me.
Thanks!
Re: Small patch to ab apr_socket_recv error handling
Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
if you want, you can commit this, the error counters are all over the
place and not really correct.
So I'm gonna keep improving ab to return the correct error stats.
Filip
Filip Hanik - Dev Lists wrote:
> ok, Jeff's feedback has been incorporated into this patch.
>
> Filip
>
> Jeff Trawick wrote:
>> On 3/2/07, Filip Hanik - Dev Lists <de...@hanik.com> wrote:
>>> is the patch below looking good?
>>> does it need adjustments?
>>> do I need to follow a different process?
>>>
>>> Filip
>>>
>>> Filip Hanik - Dev Lists wrote:
>>> > ok, final patch, this one also adds in Content-Length: 0 when keep
>>> > alive is used.
>>> > somehow, most containers will not do keep alive unless there is a
>>> > content length header.
>
> ------------------------------------------------------------------------
>
> Index: ab.c
> ===================================================================
> --- ab.c (revision 515860)
> +++ ab.c (working copy)
> @@ -258,6 +258,7 @@
> /* --------------------- GLOBALS ---------------------------- */
>
> int verbosity = 0; /* no verbosity by default */
> +int recverrok = 0; /* ok to proceed after socket receive errors */
> int posting = 0; /* GET by default */
> int requests = 1; /* Number of requests to make */
> int heartbeatres = 100; /* How often do we say we're alive */
> @@ -317,7 +318,7 @@
> #endif
>
> /* store error cases */
> -int err_length = 0, err_conn = 0, err_except = 0;
> +int err_length = 0, err_conn = 0, err_recv = 0, err_except = 0;
> int err_response = 0;
>
> apr_time_t start, endtime;
> @@ -760,8 +761,8 @@
> printf("Complete requests: %ld\n", done);
> printf("Failed requests: %ld\n", bad);
> if (bad)
> - printf(" (Connect: %d, Length: %d, Exceptions: %d)\n",
> - err_conn, err_length, err_except);
> + printf(" (Connect: %d, Receive: %d, Length: %d, Exceptions: %d)\n",
> + err_conn, err_recv, err_length, err_except);
> printf("Write errors: %ld\n", epipe);
> if (err_response)
> printf("Non-2xx responses: %d\n", err_response);
> @@ -1329,10 +1330,18 @@
> }
> /* catch legitimate fatal apr_socket_recv errors */
> else if (status != APR_SUCCESS) {
> - err_except++; /* XXX: is this the right error counter? */
> - /* XXX: Should errors here be fatal, or should we allow a
> - * certain number of them before completely failing? -aaron */
> - apr_err("apr_socket_recv", status);
> + err_recv++;
> + if (recverrok) {
> + bad++;
> + close_connection(c);
> + if ( verbosity >= 1 ) {
> + char buf[120];
> + fprintf(stderr,"%s: %s (%d)\n","apr_socket_recv", apr_strerror(status, buf, sizeof buf), status);
> + }
> + return;
> + } else {
> + apr_err("apr_socket_recv", status);
> + }
> }
> }
>
> @@ -1819,6 +1828,7 @@
> fprintf(stderr, " -S Do not show confidence estimators and warnings.\n");
> fprintf(stderr, " -g filename Output collected data to gnuplot format file.\n");
> fprintf(stderr, " -e filename Output CSV file with percentages served\n");
> + fprintf(stderr, " -r Don't exit on socket receive errors.\n");
> fprintf(stderr, " -h Display usage information (this message)\n");
> #ifdef USE_SSL
> fprintf(stderr, " -Z ciphersuite Specify SSL/TLS cipher suite (See openssl ciphers)\n");
> @@ -1981,7 +1991,7 @@
> #endif
>
> apr_getopt_init(&opt, cntxt, argc, argv);
> - while ((status = apr_getopt(opt, "n:c:t:b:T:p:v:kVhwix:y:z:C:H:P:A:g:X:de:Sq"
> + while ((status = apr_getopt(opt, "n:c:t:b:T:p:v:rkVhwix:y:z:C:H:P:A:g:X:de:Sq"
> #ifdef USE_SSL
> "Z:f:"
> #endif
> @@ -2032,6 +2042,9 @@
> exit(r);
> }
> break;
> + case 'r':
> + recverrok = 1;
> + break;
> case 'v':
> verbosity = atoi(optarg);
> break;
>
> ------------------------------------------------------------------------
>
> No virus found in this incoming message.
> Checked by AVG Free Edition.
> Version: 7.5.446 / Virus Database: 268.18.7/712 - Release Date: 3/6/2007 3:42 PM
>
Re: Small patch to ab apr_socket_recv error handling
Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
ok, Jeff's feedback has been incorporated into this patch.
Filip
Jeff Trawick wrote:
> On 3/2/07, Filip Hanik - Dev Lists <de...@hanik.com> wrote:
>> is the patch below looking good?
>> does it need adjustments?
>> do I need to follow a different process?
>>
>> Filip
>>
>> Filip Hanik - Dev Lists wrote:
>> > ok, final patch, this one also adds in Content-Length: 0 when keep
>> > alive is used.
>> > somehow, most containers will not do keep alive unless there is a
>> > content length header.
Re: Small patch to ab apr_socket_recv error handling
Posted by Jeff Trawick <tr...@gmail.com>.
On 3/2/07, Filip Hanik - Dev Lists <de...@hanik.com> wrote:
> is the patch below looking good?
> does it need adjustments?
> do I need to follow a different process?
>
> Filip
>
> Filip Hanik - Dev Lists wrote:
> > ok, final patch, this one also adds in Content-Length: 0 when keep
> > alive is used.
> > somehow, most containers will not do keep alive unless there is a
> > content length header.
That sounds very odd. Regardless, the important point for now is that
we don't want to combine two unrelated changes in one patch/commit.
The point of the patch on this discussion thread is to recover from
socket receive errors, so the patch under review/revision shouldn't
try to accomplish anything else.
> > Index: ab.c
> > ===================================================================
> > --- ab.c (revision 511976)
> > +++ ab.c (working copy)
> > @@ -258,6 +258,7 @@
> > /* --------------------- GLOBALS ---------------------------- */
> >
> > int verbosity = 0; /* no verbosity by default */
> > +int recverrok = 0;
> > int posting = 0; /* GET by default */
> > int requests = 1; /* Number of requests to make */
> > int heartbeatres = 100; /* How often do we say we're alive */
> > @@ -1330,9 +1331,19 @@
> > /* catch legitimate fatal apr_socket_recv errors */
> > else if (status != APR_SUCCESS) {
> > err_except++; /* XXX: is this the right error counter? */
> > - /* XXX: Should errors here be fatal, or should we allow a
> > - * certain number of them before completely failing? -aaron */
> > - apr_err("apr_socket_recv", status);
> > + if ( recverrok ) {
no spaces around recverrok; should be
"if (recverrok) {"
> > + bad++;
> > + close_connection(c);
> > + if ( verbosity >= 1 ) {
> > + char buf[120];
> > + fprintf(stderr,"%s: %s (%d)\n","apr_socket_recv", apr_strerror(status, buf, sizeof buf), status);
> > + }
> > + return;
> > + } else {
> > + /* XXX: Should errors here be fatal, or should we allow a
> > + * certain number of them before completely failing? -aaron */
IMO that comment can die now because of this patch.
> > + apr_err("apr_socket_recv", status);
It would be nice to slip in a message such as "Use the -r option to
continue after socket receive errors." but I don't see a trivial way
to add that in the natural message order (first the description of
what wrong, next the hint about how to take a different action when
that occurs). Punt for now unless you can think of a way to implement
that without butchering existing subroutines.
> > + }
> > }
> > }
> >
> > @@ -1559,7 +1570,7 @@
> > (posting == 0) ? "GET" : "HEAD",
> > (isproxy) ? fullurl : path,
> > AP_AB_BASEREVISION,
> > - keepalive ? "Connection: Keep-Alive\r\n" : "",
> > + keepalive ? "Connection: Keep-Alive\r\nContent-Length: 0\r\n" : "",
zap this part of the patch for now; start a discussion on that
separate issue after this patch is finished/committed
> > cookie, auth, host_field, colonhost, hdrs);
> > }
> > else {
> > @@ -1819,6 +1830,7 @@
> > fprintf(stderr, " -S Do not show confidence estimators and warnings.\n");
> > fprintf(stderr, " -g filename Output collected data to gnuplot format file.\n");
> > fprintf(stderr, " -e filename Output CSV file with percentages served\n");
> > + fprintf(stderr, " -r Don't exit on apr_socket_recv errors.\n");
IMO the usage statement should refer to "socket receive errors", not
the name of a library function
> > fprintf(stderr, " -h Display usage information (this message)\n");
> > #ifdef USE_SSL
> > fprintf(stderr, " -Z ciphersuite Specify SSL/TLS cipher suite (See openssl ciphers)\n");
> > @@ -1981,7 +1993,7 @@
> > #endif
> >
> > apr_getopt_init(&opt, cntxt, argc, argv);
> > - while ((status = apr_getopt(opt, "n:c:t:b:T:p:v:kVhwix:y:z:C:H:P:A:g:X:de:Sq"
> > + while ((status = apr_getopt(opt, "n:c:t:b:T:p:v:rkVhwix:y:z:C:H:P:A:g:X:de:Sq"
> > #ifdef USE_SSL
> > "Z:f:"
> > #endif
> > @@ -2032,6 +2044,9 @@
> > exit(r);
> > }
> > break;
> > + case 'r':
> > + recverrok = 1;
> > + break;
> > case 'v':
> > verbosity = atoi(optarg);
> > break;
bad and err_except are incremented when a receive error occurs.
Previously, that wasn't so interesting since ab aborted immediately.
IMO it is worthwhile to have a separate counter.
if (bad)
printf(" (Connect: %d, Receive: %d, Length: %d, Exceptions: %d)\n",
err_conn, err_recv, err_length, err_except);
Thanks!