On Mon, 26 Feb 2001 [EMAIL PROTECTED] wrote:
> > 3) pool_bucket_cleanup() is completely bogus AFAICT. I've added this
> > comment to the code, which describes the problems pretty well:
> > 4) The same problem applies to file buckets that have been split/copied
> > when APR_HAS_MMAP: when one of them gets read it gets made into an MMAP.
> > ALL of the file buckets should be converted at the same time to reference
> > the same MMAP.
>
> I disagree about how to fix this. The simple solution is to just abstract
> on level, such as:
>
> bucket -> bucket -> bucket
> | | |
> shared shared shared
> | | |
> -----------------------------
> |
> pool_bucket
So is what you're saying that struct apr_bucket should be a member of TWO
rings... one brigade as usual plus one ring ("brigade") of siblings
buckets that point to the same resource? I'd thought of that, but didn't
think anyone would buy it. I'm fine with the idea. But I *must* be
missing something... how does that keep us from traversing a list of
buckets when we convert types? (PS: Remember that the "shared" level is
going away.) If I've missed your point, please elaborate.
> > 5) apr_bucket_destroy() should allow the type-specific destructor to free
> > the bucket itself, too (needed to solve problem #3).
>
> No. This was done on purpose so that we can cache the buckets themselves.
> If the type-specific destructor frees the bucket itself, then we can't
> cache the freed buckets. It also makes it very difficult to convert
> buckets from one type to the next.
DOH!! Yep, you're exactly right... that won't work. Scratch #5.
> > 6) Should mmap_destroy() call apr_mmap_delete(m->mmap) after when the last
> > reference to the mmap bucket is destroyed?
>
> No. If you do that, then caching MMAP's becomes impossible. The
> apr_mmap_delete should be left up to the pool cleanup.
Okay, no problem. I'd just noticed it when I was going through the code
and thought I'd bring it up, but I wasn't married to the idea. I figured
it was left out for a reason but didn't know what it was. Scratch #6.
> > 7) socket_read() and pipe_read() should return APR_SUCCESS even in an EOF
> > condition, as discussed on new-httpd a week or so ago. I haven't made
> > this change yet, though I have put an XXX comment in the code about it,
> > because it requres fixing the input filters in Apache to figure out EOS on
> > their own. Looking for suggestions on how to do that.
>
> Didn't we decide that buckets should return EOF? I could be wrong, but I
> was under the impression that buckets needed to return EOF in order for
> things to work.
No, that's not what we decided. See message
<[EMAIL PROTECTED]> on new-httpd from Ben, which says:
----------------------------------------------------------------------
bl>>> And I'm, once more, completely confused about what should be
bl>>> happening when the socket hits EOF in blocking and non-blocking
bl>>> cases!
rbb>> IMHO it is really simple. When a socket hits EOF, it should
rbb>> simply remove the socket from the brigade. Period. That's it.
rbb>> The socket_read function should never return EOF. There is no
rbb>> such thing as EOF to a filter. Filters only know about EOS.
bl> Cool. Again, this makes perfect sense to me. It does,
bl> incidentally, have to transmute to an empty bucket, so as not to
bl> invalidate the pointer held at a higher level...
----------------------------------------------------------------------
Other messages in the thread from both Greg and myself support
this conclusion, as with one from Greg, <[EMAIL PROTECTED]>:
----------------------------------------------------------------------
bl>> ... So, the fault is that APR_EOF should _never_ be returned. ...
gs> Exactly what I said. Never return EOF. Return zero-length buckets.
gs> Don't requeue if (internally) you hit an EOF.
----------------------------------------------------------------------
Those were the last words on the matter before the subject of the thread
diverged.
> Each of these should be a separate patch, and preferably with a few hours
> in between each patch.
They are. =-)
--Cliff
