At 02:42 AM 2/25/2003, Justin Erenkrantz wrote:
[...]
I agree with 95% of all your observations, so I'll just focus on a few
discrepancies.
>--On Monday, February 24, 2003 12:21 PM -0500 Bill Stoddard <[EMAIL PROTECTED]> wrote:
>
>+ rv = read_request_line(f->next, r, b, mode);
>>+ if (rv != APR_SUCCESS) {
>>+ return rv;
>>+ }
>>+ rv = read_header_fields(f->next, r, b, mode);
>>+ if (rv != APR_SUCCESS) {
>>+#if 0
>>+ /* Handle errors at the top of the filter chain? */
>>+ if (r->status != HTTP_REQUEST_TIME_OUT) {
>>+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
>>+ "request failed: error reading the
>>headers"); + ap_send_error_response(r, 0);
>>+ ap_run_log_transaction(r);
>>+ }
>>+#endif
>
>I believe the strategy we have taken is that protocol errors
>should be passed to the output filters directly. The application
>(caller) of this filter has no idea what to do in an error.
>I think maintaining that approach would be a good thing.
What do you mean? Returning a non-apr_status_t result? We just
got finished hashing that out - since filters are APR brigade based,
then they must return a recognizable apr_status_t value. If you want
to extend that error space in the application error range, you can do
that for our filters. But you can't just return 500.
Who cares if the caller doesn't understand the error, they don't
necessarily understand 555 either. It's either good (0) or not.
>>--- server/protocol.c 3 Feb 2003 17:53:19 -0000 1.127
>>+++ server/protocol.c 24 Feb 2003 17:03:21 -0000
>>@@ -923,117 +924,16 @@
>>...
>>+ r->connection->r = r;
>>+ bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
>>+ rv = ap_get_brigade(r->input_filters, bb, AP_MODE_READBYTES,
>>+ APR_BLOCK_READ, HUGE_STRING_LEN);
>>+ apr_brigade_destroy(bb);
>
>Um, what? I think this is how you are trying to get around when to
>call the input filter. This is extremely bogus. You can't read
>data and then throw it away. And, no, setaside isn't the answer
>(sorry, OtherBill).
Huh? What setaside? Pushback, damn it - and your comments earlier
to Bill (which were correct) highlight how bogus the entire "we want to
make callers responsible for everything they fetch - stick it out so to
speak, even if they would rather have read, and would then rather just go
away, when they recognize from these bytes that they didn't belong in
the filter chain." (Paraphrasing)
The real answer is pushing back on the lower level filter so the next caller
(not the same module) reads beginning with the correct bytes.
Maybe the comprimize is that only connection level filters should support
pushback (at least initially) so that things like ssl filtering can be inserted
and removed painlessly, and the first HTTP_IN filter can also be added
or dropped for another protocol.
I've never advocated the current filter setaside approach, it buries bytes
in the wrong filters, if that's what you trying to associate me with, Justin.
>In order to even contemplating merging this, I would like to see
>a clear plan on how to support 'Upgrade' in HTTP/1.1. I imagine
>in the lifetime of 2.1/2.2, we will have a new HTTP protocol. If
>your solution doesn't allow for Upgrade, it'll have to be thrown
>away later. -- justin
We support upgrade now in httpd-2.1 .0-dev, so if we break it,
it would be obvious. Would be, had we something to test it with
in perl-framework.
I'll just close by agreeing with FirstBill's noble goals here - and point
out again that this is the time to revisit what we 'got wrong' the first
time around with our filter approach :-) Bring on the patches - but I
think we are a little ways from committing such a radical patch just
yet. I'll review in great depth the second draft, since Justin did such
a nice job with the first draft review. Hopefully trying to get these
changes right will tell us just where we went wrong with the first
filters approach.
My bottom line on the diatribe above is this; once a request's filters
are done, there should be another request just sitting in the connection
filter waiting to be read. That connection filter must be protocol agnostic
so the next HTTP_IN filter has a fresh start with no bias. Now exactly
how that happens without all the extra little cruft we've added, that Bill
is trying to mop up, is possibly best done with pushing back the unneeded
data onto the connection filter. I'll leave my comments at that and go off
to stare at my belly button and try to come up with fresh ideas.
Bill
