apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ryan Bloom <...@covalent.net>
Subject Re: cvs commit: apr-util/buckets apr_buckets_socket.c
Date Wed, 10 Oct 2001 13:59:32 GMT
On Tuesday 09 October 2001 10:44 pm, Justin Erenkrantz wrote:
> On Tue, Oct 09, 2001 at 10:35:15AM -0400, Cliff Woolley wrote:
> > -1.  By doing this, any time you do a nonblocking read from a socket
> > bucket and get EAGAIN, the whole bucket vaporizes and you never get the
> > chance to read any more data.  It was correct before: if you ask for a
> > nonblocking read, you have to be ready to handle an EAGAIN case.  So we
>
> Yes, I didn't think of that case - you are right - it probably
> explains some of the problems I was seeing with mod_ssl.
>
> I'm not sure that the bucket read code should be returning an error
> on anything but fatal errors.  And, receiving EAGAIN isn't a fatal
> error.  My take is that forcing the caller of bucket_read to handle
> EAGAIN isn't very clean.  I think this breaks the abstraction between
> sockets and buckets - I'm not sure that the caller of the bucket_read
> needs to handle EAGAIN explicitly.  Isn't part of the idea of buckets
> is to abstract out certain implementation details - the caller shouldn't
> know that it has a socket bucket underneath it?

Buckets should not be trying to handle error conditions at all.  The purpose
of buckets is to abstract out the storage mechanism, and that is what they do, 
it is not to impose our view of how code should handle errors.  By not passing
APR_EAGAIN back to the program, you are removing some very valuable
information, and making it hard to determine what is going on.  The source
code in the caller has to go from:

	do
	    do stuff so that we don't wait until there is data before
	    we process what we have now.
	while (apr_bucket_read == APR_EAGAIN)

to:
	do
	    do stuff just like above
	whild (apr_bucket_read == APR_SUCCESS &&
		APR_BUCKET_FIRST()->length == 0)

Libraries should return errors, because they are valid information that the
caller can use.  They also do not break the abstraction, because we have
essentially said that all bucket _can_ return EAGAIN, although anybody who
looks at the code will realize quickly that only socket and pipe bucket ever
_will_ return EAGAIN.

> Considering the above comments, would this patch satisfy your
> veto?  -- justin

No.   :-)

>
> Index: buckets/apr_buckets_socket.c
> ===================================================================
> RCS file: /home/cvs/apr-util/buckets/apr_buckets_socket.c,v
> retrieving revision 1.32
> diff -u -r1.32 apr_buckets_socket.c
> --- buckets/apr_buckets_socket.c	2001/10/09 05:17:18	1.32
> +++ buckets/apr_buckets_socket.c	2001/10/10 05:26:29
> @@ -77,10 +77,14 @@
>      if (block == APR_NONBLOCK_READ) {
>          apr_setsocketopt(p, APR_SO_TIMEOUT, timeout);
>          /* There was nothing to read right now, so treat it as okay and
> -         * return a 0-length brigade (see below). */
> +         * return a 0-length bucket. */
>          if (APR_STATUS_IS_EAGAIN(rv)) {
> +            free(buf);
>              *len = 0;
> -            rv = APR_SUCCESS;
> +            a = apr_bucket_immortal_make(a, "", 0);
> +            *str = a->data;
> +            APR_BUCKET_INSERT_AFTER(a, apr_bucket_socket_create(p));
> +            return APR_SUCCESS;
>          }
>      }

Don't call a function from within a macro.  You have no way of knowing how
often that function will be called.  Yes, I know it is done other places in the
code, those are incorrect too.

Ryan
______________________________________________________________
Ryan Bloom				rbb@apache.org
Covalent Technologies			rbb@covalent.net
--------------------------------------------------------------

Mime
View raw message