You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by "Roy T. Fielding" <fi...@ebuilt.com> on 2001/05/04 01:22:19 UTC
Re: cvs commit: apr/network_io/unix sendrecv.c
> Index: sendfile.c
> ===================================================================
> RCS file: /home/cvs/apr/test/sendfile.c,v
> retrieving revision 1.12
> retrieving revision 1.13
> diff -u -r1.12 -r1.13
> --- sendfile.c 2001/03/31 22:37:13 1.12
> +++ sendfile.c 2001/05/03 22:37:58 1.13
> @@ -373,6 +373,7 @@
> printf("apr_sendfile()->%d, sent %ld bytes\n", rv, (long)tmplen);
> if (rv) {
> if (APR_STATUS_IS_EAGAIN(rv)) {
> + assert(tmplen == 0);
> nsocks = 1;
> tmprv = apr_poll(pfd, &nsocks, -1);
> assert(!tmprv);
No assert should ever be present in server code, period. I don't care if
the only way it could be triggered is by a cosmic ray hittng a memory cell
at just the wrong moment in time, it doesn't belong in the server code.
Either check the error condition or be prepared to ignore it.
....Roy
Re: cvs commit: apr/network_io/unix sendrecv.c
Posted by Jeff Trawick <tr...@bellsouth.net>.
"Roy T. Fielding" <fi...@ebuilt.com> writes:
> > Index: sendfile.c
> > ===================================================================
> > RCS file: /home/cvs/apr/test/sendfile.c,v
> > retrieving revision 1.12
> > retrieving revision 1.13
> > diff -u -r1.12 -r1.13
> > --- sendfile.c 2001/03/31 22:37:13 1.12
> > +++ sendfile.c 2001/05/03 22:37:58 1.13
> > @@ -373,6 +373,7 @@
> > printf("apr_sendfile()->%d, sent %ld bytes\n", rv, (long)tmplen);
> > if (rv) {
> > if (APR_STATUS_IS_EAGAIN(rv)) {
> > + assert(tmplen == 0);
> > nsocks = 1;
> > tmprv = apr_poll(pfd, &nsocks, -1);
> > assert(!tmprv);
>
> No assert should ever be present in server code, period. I don't care if
> the only way it could be triggered is by a cosmic ray hittng a memory cell
> at just the wrong moment in time, it doesn't belong in the server
> code.
My first take was to suggest that you stop being excited about an
assert() in a test program. But then I disagree 100% with your
comments on assertions :) I practically always prefer that software
go out with a bang rather than fail silently, even when I don't have
time or don't want to complicate the logic by handling unanticipated
conditions more carefully than with an assertion.
When I used to work on mission critical code we had a fair number of
asserts, usually added when they could be done so with little extra
pathlength... They are infinitely more reliable than any comment and
they also acknowledge the fact that shit happens and you need to
figure out where things went south.
I like that Apache has asserts that only work with AP_DEBUG builds as
well as asserts that are always active. Some AP_DEBUG asserts added
by Greg Ames and others have helped us get some nasty bugs out.
--
Jeff Trawick | trawickj@bellsouth.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: cvs commit: apr/network_io/unix sendrecv.c
Posted by rb...@covalent.net.
On Thu, 3 May 2001, Roy T. Fielding wrote:
> > We have asserts throughout the server these days. Do we want to remove
> > them all?
>
> I want to remove them all, but won't do so until I've done all of the
> other things I want to do. The problem with asserts is that causing the
> server process to core dump is never a good idea, even when a person is
> in debug mode, unless the condition being checked would result in a
> fatal error in any case (like a later pointer de-ref or divide-by-zero).
> Expected run-time errors should just be logged and handled.
> In all other cases it is better to bullet-proof the code or simply ignore
> that condition as a possibility. Cosmic rays and memory errors do occur,
> so an assert should not be used unless it is a truly fatal condition.
>
> The problem with conditionally-compiled debug switches is they change
> the code such that the stuff we test isn't the same as gets deployed,
> which can mask race conditions.
>
> However, the main reason that I don't like asserts is that they lead to
> programmer mistakes -- people assuming that a condition will never occur
> in all environments simply because none of their own debug tests in their
> own environment triggered the assert. I think the code should either be
> logically complete (handle all cases) or fail-soft.
Well, you have just said what I believe in a much more concise way then I
ever could. :-) I seriously asked the question because I was hoping for
an answer like this, but couldn't voice it.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apr/network_io/unix sendrecv.c
Posted by "Roy T. Fielding" <fi...@ebuilt.com>.
> We have asserts throughout the server these days. Do we want to remove
> them all?
I want to remove them all, but won't do so until I've done all of the
other things I want to do. The problem with asserts is that causing the
server process to core dump is never a good idea, even when a person is
in debug mode, unless the condition being checked would result in a
fatal error in any case (like a later pointer de-ref or divide-by-zero).
Expected run-time errors should just be logged and handled.
In all other cases it is better to bullet-proof the code or simply ignore
that condition as a possibility. Cosmic rays and memory errors do occur,
so an assert should not be used unless it is a truly fatal condition.
The problem with conditionally-compiled debug switches is they change
the code such that the stuff we test isn't the same as gets deployed,
which can mask race conditions.
However, the main reason that I don't like asserts is that they lead to
programmer mistakes -- people assuming that a condition will never occur
in all environments simply because none of their own debug tests in their
own environment triggered the assert. I think the code should either be
logically complete (handle all cases) or fail-soft.
....Roy
Re: cvs commit: apr/network_io/unix sendrecv.c
Posted by Dirk-Willem van Gulik <di...@covalent.net>.
You two guys might violently agree :-)
Asserts - when used properly - are very usefull tools to check for
conceptional bugs, But you should always assume that you compile for
production with the -NA, -DNDEBUG, -NOASSERT or your local relevant flags.
Production code which relies on assert()s are capital bad.
As long as that is our premisse :-)
Dw
On Thu, 3 May 2001 rbb@covalent.net wrote:
>
> > > Index: sendfile.c
> > > ===================================================================
> > > RCS file: /home/cvs/apr/test/sendfile.c,v
> > > retrieving revision 1.12
> > > retrieving revision 1.13
> > > diff -u -r1.12 -r1.13
> > > --- sendfile.c 2001/03/31 22:37:13 1.12
> > > +++ sendfile.c 2001/05/03 22:37:58 1.13
> > > @@ -373,6 +373,7 @@
> > > printf("apr_sendfile()->%d, sent %ld bytes\n", rv, (long)tmplen);
> > > if (rv) {
> > > if (APR_STATUS_IS_EAGAIN(rv)) {
> > > + assert(tmplen == 0);
> > > nsocks = 1;
> > > tmprv = apr_poll(pfd, &nsocks, -1);
> > > assert(!tmprv);
> >
> > No assert should ever be present in server code, period. I don't care if
> > the only way it could be triggered is by a cosmic ray hittng a memory cell
> > at just the wrong moment in time, it doesn't belong in the server code.
> >
> > Either check the error condition or be prepared to ignore it.
>
> We have asserts throughout the server these days. Do we want to remove
> them all?
>
> Ryan
>
> _______________________________________________________________________________
> Ryan Bloom rbb@apache.org
> 406 29th St.
> San Francisco, CA 94131
> -------------------------------------------------------------------------------
>
>
Re: cvs commit: apr/network_io/unix sendrecv.c
Posted by rb...@covalent.net.
> > Index: sendfile.c
> > ===================================================================
> > RCS file: /home/cvs/apr/test/sendfile.c,v
> > retrieving revision 1.12
> > retrieving revision 1.13
> > diff -u -r1.12 -r1.13
> > --- sendfile.c 2001/03/31 22:37:13 1.12
> > +++ sendfile.c 2001/05/03 22:37:58 1.13
> > @@ -373,6 +373,7 @@
> > printf("apr_sendfile()->%d, sent %ld bytes\n", rv, (long)tmplen);
> > if (rv) {
> > if (APR_STATUS_IS_EAGAIN(rv)) {
> > + assert(tmplen == 0);
> > nsocks = 1;
> > tmprv = apr_poll(pfd, &nsocks, -1);
> > assert(!tmprv);
>
> No assert should ever be present in server code, period. I don't care if
> the only way it could be triggered is by a cosmic ray hittng a memory cell
> at just the wrong moment in time, it doesn't belong in the server code.
>
> Either check the error condition or be prepared to ignore it.
We have asserts throughout the server these days. Do we want to remove
them all?
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------