Re: cvs commit: httpd-2.0/modules/test mod_bucketeer.c
[EMAIL PROTECTED] wrote:
jwoolley2002/05/31 00:43:22
Modified:modules/test mod_bucketeer.c
Log:
we should be copying over all metadata buckets we don't understand, not
just error buckets.
Revision ChangesPath
1.12 +5 -4 httpd-2.0/modules/test/mod_bucketeer.c
Index: mod_bucketeer.c
===
RCS file: /home/cvs/httpd-2.0/modules/test/mod_bucketeer.c,v
retrieving revision 1.11
retrieving revision 1.12
diff -u -d -u -r1.11 -r1.12
--- mod_bucketeer.c 30 May 2002 21:10:19 - 1.11
+++ mod_bucketeer.c 31 May 2002 07:43:22 - 1.12
@@ -141,10 +141,11 @@
continue;
}
-if (AP_BUCKET_IS_ERROR(e)) {
-apr_bucket *err_copy;
-apr_bucket_copy(e, err_copy);
-APR_BRIGADE_INSERT_TAIL(ctx-bb, err_copy);
+if (e-length == 0) {
Looks like magic to me - perhaps wrap it in AP_BUCKET_IS_METADATA()?
Cheers,
Ben.
--
http://www.apache-ssl.org/ben.html http://www.thebunker.net/
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
Re: cvs commit: httpd-2.0/modules/test mod_bucketeer.c
Justin Erenkrantz wrote: The fact here is that not all buckets can be read such as EOS, flush, and error. Perhaps these buckets should return an error if they are read? Certainly that's not the case... all buckets must be readable. -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ A society that will trade a little liberty for a little order will lose both and deserve neither - T.Jefferson
RE: cvs commit: httpd-2.0/modules/test mod_bucketeer.c
On Fri, 31 May 2002, Ryan Bloom wrote:
-if (AP_BUCKET_IS_ERROR(e)) {
-apr_bucket *err_copy;
-apr_bucket_copy(e, err_copy);
-APR_BRIGADE_INSERT_TAIL(ctx-bb, err_copy);
+if (e-length == 0) {
Looks like magic to me - perhaps wrap it in AP_BUCKET_IS_METADATA()?
++1! This question actually came up on IRC yesterday as well, and I
told Justin that it was absolutely required. I hope to do the work
later today unless somebody beats me to it. :-)
-1.
e-length == 0 does NOT imply it *has* to be metadata. It could just be a
data bucket that's empty. On the other hand, if it's metadata that DOES
imply e-length == 0. It's a one-way relationship, not an if-and-only-if.
It turns out that most filters won't much care... if they act on data in a
bucket and the bucket contains no data, they should just pass it on along.
But it could easily be an empty HEAP bucket, and that's definitely not a
metadata bucket. If you're going to have an APR_BUCKET_IS_METADATA()
macro, it will have to test a new ismetadata flag in the
apr_bucket_type_t. But I'm -0.5 to that too, because it leads filter
authors to believe that there should be a distinction between metadata
buckets and empty data buckets when that's not the case.
--Cliff
RE: cvs commit: httpd-2.0/modules/test mod_bucketeer.c
On Fri, 31 May 2002, Cliff Woolley wrote: e-length == 0 does NOT imply it *has* to be metadata. It could just be a data bucket that's empty. On the other hand, if it's metadata that DOES imply e-length == 0. It's a one-way relationship, not an if-and-only-if. It turns out that most filters won't much care... if they act on data in a bucket and the bucket contains no data, they should just pass it on along. But it could easily be an empty HEAP bucket, and that's definitely not a metadata bucket. If you're going to have an APR_BUCKET_IS_METADATA() macro, it will have to test a new ismetadata flag in the apr_bucket_type_t. But I'm -0.5 to that too, because it leads filter authors to believe that there should be a distinction between metadata buckets and empty data buckets when that's not the case. Hmmm... okay, on second thought, +1 to the second approach. One could make the valid argument that it's safe to remove zero-length buckets if and only if they contain no metadata. How can you know which ones those are without asking the bucket itself? You can't. That requires you to pass on *all* zero-length buckets, which is a waste of time. So I'll agree to adding a field to apr_bucket_type_t to allow the bucket to indicate whether it contains metadata or not [ie, whether or not zero-length implies no useful information here]. --Cliff
Re: cvs commit: httpd-2.0/modules/test mod_bucketeer.c
On Fri, May 31, 2002 at 11:02:15AM +0100, Ben Laurie wrote:
+if (e-length == 0) {
Looks like magic to me - perhaps wrap it in AP_BUCKET_IS_METADATA()?
++1. (I'd say APR_BUCKET_IS_METADATA.) -- justin
RE: cvs commit: httpd-2.0/modules/test mod_bucketeer.c
From: Justin Erenkrantz [mailto:[EMAIL PROTECTED]] On Thu, May 30, 2002 at 02:43:05PM -0700, Ryan Bloom wrote: This doesn't fix the underlying problem. Mod_bucketeer needs to be smart enough to copy buckets if it doesn't recognize their type. If the bucket can't be copied, then the filter should remove the bucket from one brigade and add it to the other. Any filter that silently removes buckets is broken. True, but I thought part of the idea behind buckets is so that the backing type doesn't have to be known. Exactly! The fact here is that not all buckets can be read such as EOS, flush, and error. Perhaps these buckets should return an error if they are read? Then, if a filter sees APR_ENOTIMPL, it can copy them over. Every bucket can be read. That is a mandatory function that all buckets must implement. If the back-end bucket-type isn't know, the filter can ALWAYS do a read/apr_bucket_make_heap. Ryan
Re: cvs commit: httpd-2.0/modules/test mod_bucketeer.c
On Thu, 30 May 2002, Justin Erenkrantz wrote: The fact here is that not all buckets can be read such as EOS, flush, and error. Sure they can. They just contain zero data bytes. Perhaps these buckets should return an error if they are read? Then, if a filter sees APR_ENOTIMPL, it can copy them over. -- justin No way. *All* buckets can be read from, that's an important part of the design. Having to test for APR_ENOTIMPL all over the place would be a major drag. --Cliff
RE: cvs commit: httpd-2.0/modules/test mod_bucketeer.c
On Thu, 30 May 2002, Ryan Bloom wrote: True, but I thought part of the idea behind buckets is so that the backing type doesn't have to be known. Exactly! Exactly. All you really need to test here is length. If the bucket is zero bytes, that's all you need to know. That gets you all metadata buckets handled in one fell swoop. If there are certain kinds of metadata buckets a filter *does* know about and want to handle specially (eg EOS or FLUSH), it can make the choice to special case those. Every bucket can be read. That is a mandatory function that all buckets must implement. If the back-end bucket-type isn't know, the filter can ALWAYS do a read/apr_bucket_make_heap. True. Though we've specified the additional semantics that apr_bucket_read() causes the bucket to auto-morph into one that is in-memory. Either way. --Cliff
Re: cvs commit: httpd-2.0/modules/test mod_bucketeer.c
On Thu, May 30, 2002 at 05:50:45PM -0400, Cliff Woolley wrote: On Thu, 30 May 2002, Justin Erenkrantz wrote: The fact here is that not all buckets can be read such as EOS, flush, and error. Sure they can. They just contain zero data bytes. Right, but the problem is how are these metadata buckets identified if their types aren't checked for explicitly? It'd be nice if we came up with a way to prevent every module for checking for EOS, flush, and error each time. -- justin
RE: cvs commit: httpd-2.0/modules/test mod_bucketeer.c
Every bucket can be read. That is a mandatory function that all buckets must implement. If the back-end bucket-type isn't know, the filter can ALWAYS do a read/apr_bucket_make_heap. True. Though we've specified the additional semantics that apr_bucket_read() causes the bucket to auto-morph into one that is in-memory. Either way. I didn't think it _had_ to auto-morph. My understanding is that the default buckets do, because we assume the performance will be better if they do. That makes sense, because a file_bucket is likely to be read multiple times, so it makes sense to morph the bucket on the first read, so that we don't have to go to the disk twice. Ryan
RE: cvs commit: httpd-2.0/modules/test mod_bucketeer.c
On Thu, 30 May 2002, Ryan Bloom wrote: I didn't think it _had_ to auto-morph. My understanding is that the default buckets do, because we assume the performance will be better if they do. That makes sense, because a file_bucket is likely to be read multiple times, so it makes sense to morph the bucket on the first read, so that we don't have to go to the disk twice. If it didn't, how could you possibly pass back a buffer containing the data? In other words, I guess there's no hard and fast rule that it *has* to actually morph, but since it's doing all the work of copying that data into an in-memory buffer anyway, it doesn't make much sense *not* to morph to a bucket type that handles in-memory buffers. Sure it could allocate a new buffer and redo the copy every single time it was read from, but why would it want to? shrug --Cliff
RE: cvs commit: httpd-2.0/modules/test mod_bucketeer.c
From: Justin Erenkrantz [mailto:[EMAIL PROTECTED]] On Thu, May 30, 2002 at 05:50:45PM -0400, Cliff Woolley wrote: On Thu, 30 May 2002, Justin Erenkrantz wrote: The fact here is that not all buckets can be read such as EOS, flush, and error. Sure they can. They just contain zero data bytes. Right, but the problem is how are these metadata buckets identified if their types aren't checked for explicitly? It'd be nice if we came up with a way to prevent every module for checking for EOS, flush, and error each time. -- Justin All of the metadata buckets have a common interface. They all have a 0 length. So, filters should be written so that any 0 length bucket is just moved from one brigade to the next. If 0 length buckets are removed from the brigade, then the filter is most likely doing something wrong. The only time this isn't true, is when the bucket is a 0 length heap/mmap/file/transient bucket, but those don't really matter, because the lowest level filters will remove those cleanly. Since the lowest level filters know which are the metadata buckets, they also know that any 0 length bucket that isn't a metadata bucket can be removed. Ryan
RE: cvs commit: httpd-2.0/modules/test mod_bucketeer.c
From: Cliff Woolley [mailto:[EMAIL PROTECTED]] On Thu, 30 May 2002, Ryan Bloom wrote: I didn't think it _had_ to auto-morph. My understanding is that the default buckets do, because we assume the performance will be better if they do. That makes sense, because a file_bucket is likely to be read multiple times, so it makes sense to morph the bucket on the first read, so that we don't have to go to the disk twice. If it didn't, how could you possibly pass back a buffer containing the data? In other words, I guess there's no hard and fast rule that it *has* to actually morph, but since it's doing all the work of copying that data into an in-memory buffer anyway, it doesn't make much sense *not* to morph to a bucket type that handles in-memory buffers. Sure it could allocate a new buffer and redo the copy every single time it was read from, but why would it want to? shrug Agreed, but I can think of one case. (a bit of hand-waving, but a possibility, nonetheless) :-) If I write two filters that work in tandem to solve a problem, I may want to use a special bucket type to facilitate them working together. For example, if I have a bucket type that encodes graphics in lists of RGB values, then the read function MUST return a binary buffer of data, but later filters will most likely find it MUCH easier to deal with the RGB values directly. So, in this case, I could have a handler that served graphics files by filling out this special bucket, and a filter later on that added a watermark. Any filter in between the two will still have to be able to read from the bucket, but the watermark filter will be able to operate much better on the RGB values. That is the only type of case that would make sense to keep the data in a different structure. :-) Ryan
RE: cvs commit: httpd-2.0/modules/test mod_bucketeer.c
On Thu, 30 May 2002, Ryan Bloom wrote: Agreed, but I can think of one case. (a bit of hand-waving, but a possibility, nonetheless) :-) If I write two filters that work in tandem to solve a problem, I may want to use a special bucket type to facilitate them working together. Fair enough. I was thinking of a similar situation a few minutes ago. Though I think in practice it's likely that such a bucket type would hang on to the buffer it copied the data into in addition to the native format and would therefore meet my earlier assertion by virtue of having the data in both formats (binary buffer and native format) after the first call to apr_bucket_read(). It wouldn't *have* to, but it probably would anyway. :) --Cliff
Re: cvs commit: httpd-2.0/modules/test mod_bucketeer.c config.m4
On Sat, Feb 23, 2002 at 06:43:25AM -, [EMAIL PROTECTED] wrote: ianh02/02/22 22:43:25 Modified:modules/test config.m4 Added: modules/test mod_bucketeer.c Log: introducing the bucketeer it lets you set up boundry test conditions for things like mod-include snip 1.1 httpd-2.0/modules/test/mod_bucketeer.c Index: mod_bucketeer.c === snip * Portions of this software are based upon public domain software * (zlib functions gz_open and gzwrite) */ I take it that this comment is stale? -- justin
