httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ryan Bloom <>
Subject Re: cvs commit: apache-apr/pthreads/src/main http_protocol.c
Date Wed, 03 Mar 1999 04:45:29 GMT
I just reviewed the code, so I'll try to explain what I was doing, and

I got a report that the timeout code was causing problems with not serving
requests after the first seven or so.  After investigating, I found out we
stopped serving requests after the first X, where X is the number of
worker threads.  This ended up being because reqad_request_line wasn't
resetting errno in some cases.  I honestly can't remember which ones,
because this was written last week sometime.

> First it's almost never approprate [1] to set errno to zero.  It's not even in
> the errno's value set.  

That's why I set it to zero.  My thinking was to reset errno to a value
that no function should ever have returned.

> Second it's almost never approprate to try and get errno to hold a value for
> more than a few instructions.  It's absolutely hopeless to call a function and
> expect it to survive the call.  For example ap_bfileno might have dubug or
> loging hooks in it and these might have errno setters in thier recursive decent.

I'm probably missing something down some levels in the internal apache
calls.  But I don't see where I am doing this.  I call read_request_line
to set errno, and check it immediately.  After that, I cleared it, just to
be sure it wasn't set to EAGAIN the next time I got to the if statement,
unless read_request_line was the function that set it.

> Having had my interest drawn to this code...  It looks pretty racing assuming
> that read_request_line left anything useful in errno when it returns zero.  Very
> careful inspection of the code is probably necessary to be absolutely sure.  It
> doesn't look very safe to me...

It has worked in all of my tests, and a very quick look at the code seemed
to be okay.  I will look closer tonight.  I honestly never expected this
code to stay here forever.  I would rather either use NSPR's read, which
has a timeout value, or write an APR function that has a timeout.  Both
options require that multiple functions be changed, and I don't want to
lock us in until a decision has been made.  I also wanted a quick solution
that would get us moving again, and I had every intention of coming back
to it.  Hopefully, I'll get to look at it again tonight or tomorrow.

> read_request_line might return the errno from getline.
> getline might be returning the errno that from ap_bgets.
> The behavior of ap_bgets is a mess in the -1 return case.  
> It sets errno only on some of it's -1 return paths.
> Bleck.
> I think that the buff routines signal error by:
>  1. Invoking the installed call back.
>  2. Marking the buffer as having had an error.
>  3. Returning an exceptional value, but _not_ setting errno.
> Hope this is useful.

Very.  I'll look into this stuff again tonight, but if you see another way
to solve the problem, let me know.

>  - ben
> [1] The only case I know of where it is approprate to set it to zero
> is when you are debugging and your trying to get some hint if anything
> "exceptional" happened recently.

We actually check for errno == 0 in http_log.  Well, that's not quite
true.  We set save_errno to errno, and check it for zero (search for
NOERRNO), so I assumed I was okay to do this.


Ryan Bloom
4205 S Miami Blvd	
RTP, NC 27709		It's a beautiful sight to see good dancers 
			doing simple steps.  It's a painful sight to
			see beginners doing complicated patterns.	

View raw message