httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: Patch review: Re: [PATCH] hook based filters take 4
Date Tue, 27 Jun 2000 09:19:35 GMT
On Mon, Jun 26, 2000 at 08:14:51PM -0700, rbb@covalent.net wrote:
> 
> I posted a bit quickly, because I was in a hurry, I'll fix the big
> issues tonight and re-post.  I don't remember half of this e-mail.  I
> wrote and re-wrote all of it three times.  If I offend, sorry.
> 
> You're patch has other issues.
> 
> 
> 1) The first is that you send headers before ANY module has filtered any
> data.

I described the header stuff, in detail, in the review of your patch. What
you did with headers applies exactly the same to my patch, but I simply
deferred that point until it could be completed across the entire code base.

I'm not going to repeat more details here. Go see the patch review, down
under the "good" section where I discuss the delay of issuing headers.

This is not an issue.

> 2) We cannot allow people to pass char *'s through filters.  It removes
> optimizations, because it loses information about where the data comes
> from.  I have yet to come up with a filter design that cannot be
> implemented better with the bucket design.

We certainly can pass char* through. Why? One simple fact: there are some
filters that must examine *EACH* and *EVERY* character that passes through
them.

What does that filter want? You got it. char*

You give it a file, it maps it or reads it to turn it into a char*. You give
it a va_list of char* pointers? It processes each one. Give it a printf? It
formats it and processes each character.

To state that a filter cannot consume only a char* is ignoring the obvious.

You believe that your buckets are the answer, but that is only a partial
truth. Your example filter is missing the step that transforms the provided
bucket (whatever it may be) into a char* for processing. I put that into the
framework so that filters don't have to deal with it.

Optimizations? If you have to examine every character, there isn't a whole
hell of a lot that you can do. It has to happen.

Optimizations when you *don't* have to examine every character. Fine. Use
the bucket_cb in my patches. If you don't mind the extra work.

By placing the transformations (of the various inputs into a char* for a
simple_cb) into the filter mechanism, I can apply quite a few more
optimizations. Heck, I don't force alloc on every ap_rwrite (and friends).
That is a heck of a lot more optimal.

Optimizations? If all the filters are using the bucket_cb, it is entirely
possible that whatever data is in that bucket is *not touched* by the
filters. See the header/footer example in my mod_gargle. It passes the
bucket right along, untouched. The implication is that whatever is passed to
ap_rwrite() and friends shoots right down to the network, untouched,
unprocessed, and with zero allocs or memcpys. Fact.

> 3) The ordering of filters is impossible in your scheme.

FUD. As I've stated before, the ordering can be done in one of two ways:

1) make use the ordering on the "insert_filter" hook. If a module wants to
   insert theirs at the end, then they can use AP_HOOK_LAST. If the core
   wants to be the very last, its insert_filter can be AP_HOOK_REALLY_LAST.
   When that hook adds filters to the request, they are always added at the
   end. The effect is that the first hook called is the first to add a
   filter. The last hook called adds the last filter.

2) since each filter is named in my scheme, it is possible to use something
   like SetFilter to specifically outline the sequence that should be used.

> You refuse to
> accept that modules will want to insert themselves in the middle of the
> request processing.

Don't put words in my mouth.

I totally agree that they must be ordered. Shit. I was pushing that simple
fact six weeks ago trying to convince *you* that they must be ordered. Don't
think for a minute that I believe they shouldn't be orderable.

That said, I've already outlined a scheme for responding to changes in the
content type. It is a simple mapping from content type to filter name for
insertion.

My filters are typed (for this eventuality, and for the subrequest issue
regarding connection filters), so it already possible to do the insertion at
the end of the CONTENT filters, but before the CONNECTION filters. I'll add
the condition to the "while" loop in ap_add_filter() to demonstrate this.

I've stated that I don't believe additional classifications (beyond the two)
are possible, but if somebody can show the requirement, then the classes can
be inserted into ap_filter_type. That also happens to produce a class-based
ordering.

> They will want to.  Admins will not want to have to
> detail which filters operate on which CGI's.  This scheme requires that
> they do.

No. My filters are named. I can set up a mapping from MIME type to filter
name. I have outlined this before.

Your patch does not address this issue either, so it isn't right to claim a
failing in mine. However, I *do* have the necessary infrastructure to
support this concept. That is a win.

Scrolling back... (3) is an issue only insofar that an extra condition in
ap_add_filter() will appease the problem:

<                while (fscan->next != NULL)
<                    fscan = fscan->next;
--
>                while (fscan->next != NULL
>                       && fscan->next->ftype <= f->ftype)
>                    fscan = fscan->next;
>                f->next = fscan->next;

Done. Ordering.

> > *) the NEXT_FILTER design allows you to send data to the next filter *ONCE*.
> >    You cannot deliver content several times. This implies that a filter must
> >    gather all the content that is to be passed along to the next filter.
> 
> This is a non-issue.  This is incredibly easy to fix, and will be in my
> new patch.  This is a result of me posting too quickly.

Great. I see this fixed in your latest patch.

[ well, one bug: your NEXT_FILTER macro still has a "return" in it ]

> > *) a bit related to above, I would like to hear how the subrequests would be
> >    handled (in terms of feeding subreq content into the "next" filter).
> >    
> >    Conversely: I've described how my patch set will do this, but it probably
> >    has not been clear. I will reissue the patch with an example of this.
> 
> I still do not see the issue here.

Lets say that your filter invokes a subrequest. How do you get that
subrequest's content into the filter chain?

Consider that you have a chain set up, where Fn are the CONTENT filters and
Cn are the CONNECTION filters:

    F1  ->  F2  ->  C1  ->  C2

Now, let's say that F1 issues a subrequest to generate some content inline.
The filter chain for the subrequest must look like:

       F1  ->      F2  ->  C1  ->  C2
         \        /
subreq:   -> S1 -> 

In my scheme, I simply say: 

    subreq = ap_sub_req_lookup_uri(uri, filter->r);
    subreq->filters = filter->next;
    if (ap_run_sub_req(subreq) != OK) {
       ap_log_rerror(...)
       return;
    }

I do not understand how a similar setup is constructed in the hook
situation.

> > *) because NEXT_FILTER directly invokes the next filter in line, there is no
> >    way to short-circuit filter processing when the client has dropped the
> >    connection (or some other error has occured and aborted the connection).
> >    
> >    Conversely: ap_l*() all check for an aborted connection before passing
> >    the content to the next filter.
> 
> This too is incredibly easy to add.  Expect this in the next patch.

I see that you updated NEXT_FILTER with an "aborted" test. Okay.

> > *) All the "output" routines used by a content generator (ap_rputc(),
> >    ap_rputs(), ap_rwrite(), ap_vrprintf(), ap_rprintf(), and ap_rvputs())
> >    requires two allocations to be performed (to construct an ioqueue *and*
> >    an ioblock). If the content generator makes 1000 calls to ap_rwrite(),
> >    then you have 2000 allocations which are not freed until the end of the
> >    request.
> 
> If the content generator is will written, this isn't true.  It will
> require one allocation for the ioqueue, and one each for the ioblock.

What the hell? Let's pull out the handy old mod_autoindex for an empirical
case study in how a content generator is truly written. Let's just consider
the output_directories() function:

    17 calls to ap_rputs()
    2 calls to ap_rprintf()
    5 calls to ap_rvputs()

Half of those are in a loop, one per file in the directory.

Each of those invocations will require an ioqueue alloc and an ioblock
alloc. Let's say that we have a directory with 100 files in it, and let's
assume half of the 24 ap_r*() calls are inside the loop. That is 12 calls
for 100 files, each doing 2 allocations. That is 2400 allocations when you
visit a 100 file directory.

I consider that a problem. *Especially* because they are performed whether
or not a filter is installed.

I also would like to point out that my patch has zero allocations in this
scenario, if no filters are installed. If the filter is using simple_cb,
then it will do one allocation for each ap_rprintf() call so that the
callback can examine the resulting printf characters. If the filter is using
bucket_cb, then I have zero allocation overhead again. [the filter may
cause alloc to process data, but that is outside the scope here]

> > *) ap_vrprintf() and ap_rprintf() generate the content onto the heap
> >    unconditionally. My patch set will avoid this when buckets are used.
> 
> Please explain how you expect modules to be able to deal with a format
> string and arguments.  Unless you provide a function for modules to get an
> actual string from the bucket type, this is useless as far as I can see.

By passing through the fmt and va_list in my ap_bucket_t, I stand the
possible chance that the heap allocation will never occur. As a concrete
example, consider the header/footer in my mod_gargle. It will not cause the
v/rprintf result to be shoved onto the stack. It ends up going down to the
network as a fmt/va pair. The BUFF routines then use the printf formatter
function to shove the result directly into the BUFF buffer, for placement
onto the wire.

Unconditionally placing it onto the heap means:

1) you consume heap space for each one of these calls
2) when the formatted string hits the BUFF, it will be memcpy'd into the
   BUFF's buffer

Not all is lost, though :-) ... you simply need to place the fmt and va_list
into your ioblock. If somebody needs the characters, then they can figure
out how to get them. (I do it for them in simple_cb, but let them deal with
the bucket_cb scenario)

> > *) nit: bug in ap_rputc(): &c is not a valid char ptr -- endian issues might
> >    point to the wrong byte.
> 
> Also, easy to solve, expect it gone in the new patch.

I see that. Note that you could have just declared "char c2 = (char)c" and
pointed your ioblock at that. No need to do another heap allocation.

> > *) The "keep_around" design isn't fully implemented, so I can only guess
> >    that the code is reserving space for the ioqueues (which are passed
> >    through the filters) to be set aside into those slots.
> > 
> >    *Presuming* that is the design, then it *can* work, but would be very
> >    prone to bugs. ap_rputc() is a perfect demonstration of this: the pointer
> >    which is stored into the ioblock points to the stack. Upon return from
> >    ap_rputc(), this pointer will be invalid. When that queue/block is fed
> >    back in on the next invocation, the filter will receive bad data.
> > 
> >    Conversely: my patch doesn't provide any facility for keeping data. It is
> >    up to the filter to determine how to do that best, and to manage the
> >    lifetime of that appropriately. Admittedly, I can't have any bugs in the
> >    algorithm since it doesn't exist :-)
> > 
> >    Basic point: keep_around cannot work unless copies are made of all
> >    ptr/len pairs stored in the ioblocks.
> 
> Wrong, it can work.  This too will work in the next patch.

Not wrong. I said right up front that I was guessing at where this design
was headed.

In your latest patch, I see that you torched keep_around and went with
another scheme. Great.
[ it still has problems, but I'll save that for a review of your latest
  patch ]

> > *) mod_gargle, as implemented, is an ineffective demonstration of filtering.
> >    It has three main issues:
> > 
> >    1) There is no consideration for anything but a ptr/len pair. If a true
> >       "bucket" is being passed (which this patch *always* does), then there
> >       should be considerations for the other bucket types.
> 
> Why?  Currently, I only have files and ptr/len pairs.  Files are
> implemented through mmap'ing the file and passing a ptr/len
> pair.  Mod_gargle is fine in this respect.

My point is that mod_gargle is making no consideration for the fact that a
file might be in the ioblock. At a minimum, it should call a utility
function which will return the ptr/len pair. Of course, you're going to have
some fun when mmap isn't available. What then? Suck it all onto the heap?
Obviously, that can't be done, so the utility function will need to say
"here is a chunk, call me again to get some more". Or maybe the filter will
understand that a file is in there and will do the ap_read() loop itself.

In my patch, the simple_cb will be called with the mmap/len pair if mmap is
available. If not, then simple_cb will be called for each block read in the
ap_read() loop. If a bucket_cb is used, then the filter can deal with it
(and have the same problems that I outline above for your patch).

The code in my patch to do the file handling is not present, per my earlier
notes about deferring that to get the ap_send_fd() handling correct. But
when that *does* happen, I won't have to bungle up the filters. They're
already set.

Point is: mod_gargle is assuming it is receiving only ptr/len pairs. A
proper example of your scheme will have it deal with the files that might
occur in an ioblock.

[ side note: per my suggestion above, you will also want a fmt/va pair in
  your ioblock. mod_gargle will need to deal with that, too. ]

> >    2) It only gargles individual data elements which are longer than 10
> >       bytes. While this is quite valid as a functional description, it is
> >       inadequate for an example. If the content generator only spits out
> >       single characters, then nothing will be gargled. Placing two gargle
> >       filters in a row is non-indempotent. The first will split the ioblocks
> >       into single character blocks, or 9-char blocks. The second gargle will
> >       do nothing.
> 
> I decided not to implement the full thing.  I fix this in the new patch
> too.  I assumed this was simple to do, so I didn't bother.  Sorry.

No need to apologize. I'm just seeking a working, *interesting* example of
filtering. Your "gargle" was interesting, if I assumed that you meant to
change every 10th/11th char in the file.

I see that you updated the function in your latest patch. It has bugs, and
could even cause the dropping of the entire file. Presuming that you want a
working example, I can provide a detailed bug report. I'll defer it for now
because I feel like I'm working on two sets of patches :-)

> >    3) I would also like to see an example of how to do a header/footer. This
> >       requires a private flag to the filter instance, to remember whether it
> >       has sent the header. I would also advocate an example that doesn't use
> >       r->bytes_sent to determine this -- the private flag might be set at
> >       install-time to prevent a header and only send a footer.
> > 
> >    4) Yes, I'm being a pain :-) ... I'd also like to see a subrequest
> >       example where the subrequest output is filtered.

These two requests were made so that we can see how your scheme operates in
different filtering scenarios.

I do see that you added some macros to use r->request_config for holding
unused input. There isn't any assistance at this point for arbitrary state.
The scheme also has an inherent problem. Comments to that elsewhere.

> > *) confusion between the different sets of hooks.
> > 
> >    Consider mod_gargle.c: AP_HOOK_FILTER is used for the global filter.
> >    That should be AP_HOOK_MIDDLE or something.
> 
> It should not, it should be AP_HOOK_FILTER.  Read the docs in the patch
> please.  This is a content filter, so it should be AP_HOOK_FILTER

Read my comment, please :-) ... I referred to the "global filter". The one
set in mod_gargle's register_hooks() function. That should be
AP_HOOK_MIDDLE.

The hook insertion in gargle_insert_filter() should use AP_HOOK_FILTER as
you rightly point out, and it does.

The fact that you were confused on this issue, despite my calling it out,
seems to validate my cause for concern. How can we expect other developers
to keep the hook ordering values straight, when its primary author is mixing
and matching them?

> >    Consider http_core.c: AP_HOOK_REALLY_LAST is used for its filter, but
> >    that should be AP_HOOK_TRANSPORT or something.
> 
> Actually, this should be AP_HOOK_REALLY_LAST.  The core is a special case,
> because it is actually writing to the network.  If I use TRANSPORT, then
> we have to rely on the other module to say that http_core is run after
> it.  This removes that onus from the module author.

I forget where I mentioned it, but this is dangerous. REALLY_LAST and
TRANSPORT have identical, numeric values. If somebody registers a TRANSPORT
filter, it could end up after the core.

Ideally, you want another symbol similar to REALLY_LAST, but larger in value
than TRANSPORT.

> > *) ap_bwrite_block() is flawed in that it attempts to optimize around BUFF,
> >    but fails drastically. Consider the LARGE_WRITE_THRESHOLD test at the
> >    beginning of the function. If there is a lot of (total) data, it decides
> >    that it won't fit in the buff, so it is best to just write it all to the
> >    network. Unfortunately, it drops each ioblock chunk onto the net. If I
> >    have a large document that was run through mod_gargle (producing
> >    thousands of 1 and 9 byte ioblocks), then I'll meet the threshold test,
> >    and then proceed to generate a bazillion packets (one for each
> >    iol_write() call). On Linux, with TCP_CORK, that's no biggy. Elsewhere?
> >    Oof.
> 
> This can be easily solved by using iovec's and writev.

I can believe this, but I am concerned about the changes that you're making
inside buff. I think it is too early/quick to do that. You've introduced
problems (e.g. the writev thing, not flushing the buffer), lost the xlate
stuff, lost the chunked transfer coding, and may be creating unknown
problems in there. Ever since I joined this mailing list, people have feared
the buff.c code and try to avoid tickling it too much for fear of
introducing bugs. (not to mention Dean's wrath :-)

> > *) all the calls to check_first_conn_error() have been removed. As a result,
> >    the connection will never be aborted when an error occurs. No message
> >    will be logged explaining that.
> 
> They have not been removed.  Please review the patch.  They are in
> SETUP_FILTERS.  The code was repeated all over the place, I tried to
> simplify fixing bugs later.  Sorry.

Sigh. 

I'm talking about check_first_conn_error(). Not the ->aborted test. You
eliminated all calls to check_first_conn_error when an error occurs. As a
result, ->aborted will never be set (on error) and a message will not be
logged.

> > *) keep_around is tied to the number of filters on the first write. Consider
> >    the case where we have PHP installed as a filter, and it generates
> >    content with MIME type "text/x-server-parsed-file". The SSI filter
> >    installs itself at that point, to deal with processing the output. Oops.
> >    There are now two filters, one keep_around entry.
> >    
> >    Conversely: in my patch, the context is associated directly with each
> >    installed filter (ap_filter_t.ctx)
> 
> >      In my post on this matter, I described how the overall "insert_filter"
> >      could be REALLY_LAST. Thus, the core is the last thing to be called to
> >      insert a filter, and it calls ap_add_filter() on its filter. Since that
> >      is the last add_filter to be called, the filter goes to the end.
> 
> But you still don't allow more modules to insert filters at later
> processing stages.  This is a MUST.

See my post above. This isn't an issue for my patch, given the names and the
ftype.

I think that you're going to have a harder time specifying how filters are
inserted when changes occur (content-type or other). There are no names, so
there is no way to set up mappings. As a result, each filter is going to
have hard-coded knowledge about when it must insert itself. Presumably,
you'll run another hook when the content type, et al, changes value.


Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Mime
View raw message