On 3/2/07, Filip Hanik - Dev Lists <[EMAIL PROTECTED]> 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!
