httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jeff Trawick" <traw...@gmail.com>
Subject Re: Small patch to ab apr_socket_recv error handling
Date Sat, 03 Mar 2007 12:33:53 GMT
On 3/2/07, Filip Hanik - Dev Lists <devlists@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!

Mime
View raw message