httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ben Laurie <...@algroup.co.uk>
Subject Re: socket_read?
Date Sun, 18 Feb 2001 11:23:06 GMT
Cliff Woolley wrote:
> 
> > -----Original Message-----
> > >     if ((rv=apr_bucket_read(e,&s,&len,APR_BLOCK_READ)) != APR_SUCCESS)
> > >         return rv;
> 
> > Break in what sense? It will cause them to return EOF when they read an
> > EOF. Is that broken? If you carry on reading past an EOF what is the
> > expected behaviour from the underlying filter? Surely once its returned
> > EOF its game over?
> 
> Nope--at least not always.  This particular snippet is from
> apr_brigade_partition().  That function loops through the entire brigade one
> bucket at a time, trying to ensure that there's a bucket boundary at a given
> byte offset into the brigade.  It avoids reading buckets as best it can, but
> this can't be avoided for pipe and socket buckets, which have an indeterminate
> length before being read.  APR_EOF within apr_bucket_read() of a pipe or socket
> bucket just means "you've read all there is to read, this bucket is now empty."

Certainly in the case of a socket, a blocking read will _not_ return
APR_EOF (this was my original complaint!). So I'm not sure what you mean
by this.

> But that's a success case, at least as far as apr_brigade_partition is
> concerned.  Consider a pipe bucket in the middle of a brigade being passed down
> the chain from a CGI or something similar.  There very well might be buckets
> after the pipe bucket.  Just because the pipe bucket's empty, that doesn't mean
> we abort from apr_brigade_partition, it just means we ignore it and move on to
> the next bucket.

Hmm. Good point. So what should happen in _this_ case is that the pipe
bucket should be deleted from the brigade. Hmm. And then you end up with
the possibility of having no bucket. OK, I begin to see the logic, but I
still don't like it: you return a zero length bucket simply because you
have to return something.

But this is an artificial kludge. It makes just as much sense to me to
return no bucket at all and an error code (even if its something other
that EOF), and its more efficient.

> > Of course, the right answer may be that socket_read shouldn't return EOF
> > at all, it should return an EOS bucket.
> 
> Nooooo... definitely not.  You'd easily end up with multiple EOS buckets in the
> brigade if socket_read() did that, which is very bad (plus you'd break the above
> even worse).  As Jeff pointed out, an EOF condition in a particular bucket may
> or may not represent the end of the entire brigade (which is what an EOS bucket
> is for).

OK, you are right, I hadn't thought that through.

> > But that's a different matter -
> > what I'm currently trying to establish is that it should do the same
> > thing regardless of whether it is blocking or not.
> 
> That might be.  I'd personally argue that they're both success cases... you've
> read as much as there is to read from this bucket.  Move on to the next bucket.

Exactly.

> > I actually suspect it is doing the wrong thing in both cases currently
> > (i.e. empty bucket in one,
> 
> Yep.  In this case, the pipe/socket bucket should just remove itself from the
> brigade, to avoid trying to re-read it on another pass through the brigade.
> That's easy enough to fix.

The problem is that the bucket transforms itself into the read data, and
then (if there's more to come) appends a new socket bucket. Deleting
itself would invalidate the bucket pointer in the caller - which would
be fine if we returned an error code to indicate that we had done so
(APR_BUCKET_FINISHED?).

> > APR_EOF in the other, instead of EOS bucket
> > in both), so once we've all agreed the first point, let's figure this
> > one out, too!
> 
> We could make both of them return APR_SUCCESS, which makes sense for
> type-agnostic callers that just want to know nothing catastrophic happened and
> that they can move on to the next bucket in the brigade.

I still say its bizarre to return an empty bucket. Which you have to do
in this case.

> We could return APR_EOF for both, that might make sense for callers that know
> about different kinds of bucket.  But I just don't see how it does the caller
> any good... the EOS bucket at the end of the brigade should be all that's cared
> about, right?  What am I missing?  I'm going to go look more closely at mod_tls
> to see if this makes more sense to me...

What you are missing, I think, is that you can't delete the bucket _and_
return SUCCESS.

> Bottom line: I don't have a problem with making them both return the same value,
> whatever value that is... just no EOS buckets, please.  =-)

Agreed.

> PS: Jon Travis is the one that originally submitted the patch to make it return
> APR_EOF in one case and APR_SUCCESS in the other... maybe he'd care to comment
> on this?

That might help.

Cheers,

Ben.

--
http://www.apache-ssl.org/ben.html

"There is no limit to what a man can do or how far he can go if he
doesn't mind who gets the credit." - Robert Woodruff

Mime
View raw message