You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by br...@apache.org on 2005/10/09 03:37:14 UTC
svn commit: r307339 - in /httpd/httpd/branches/async-dev: CHANGES
include/http_request.h modules/http/http_core.c modules/http/http_request.c
server/Makefile.in server/eor_bucket.c
Author: brianp
Date: Sat Oct 8 18:37:11 2005
New Revision: 307339
URL: http://svn.apache.org/viewcvs?rev=307339&view=rev
Log:
Redesign of request cleanup:
- A new End-Of-Request bucket is pushed through the output filter
chain after the last bucket of the response.
- This bucket gets destroyed by ap_core_output_filter() after the
buckets in front of it have been sent.
- The destroy callback of the EOR bucket invokes the access logger
and frees the request's pool.
With this change, the request logger now runs after the last byte of
the response is _sent_, rather than after the last byte of the response
is _generated_. This should make the bytes-sent count in the access
log more accurate in cases where the client closes the connection
midway through the sending of the response.
Added:
httpd/httpd/branches/async-dev/server/eor_bucket.c
Modified:
httpd/httpd/branches/async-dev/CHANGES
httpd/httpd/branches/async-dev/include/http_request.h
httpd/httpd/branches/async-dev/modules/http/http_core.c
httpd/httpd/branches/async-dev/modules/http/http_request.c
httpd/httpd/branches/async-dev/server/Makefile.in
Modified: httpd/httpd/branches/async-dev/CHANGES
URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/async-dev/CHANGES?rev=307339&r1=307338&r2=307339&view=diff
==============================================================================
--- httpd/httpd/branches/async-dev/CHANGES [utf-8] (original)
+++ httpd/httpd/branches/async-dev/CHANGES [utf-8] Sat Oct 8 18:37:11 2005
@@ -1,6 +1,12 @@
-*- coding: utf-8 -*-
Changes in Apache 2.3.0 async-dev R&D branch
+ *) Added an End-Of-Request bucket type. The logging of a request and
+ the freeing of its pool are now done when the EOR bucket is destroyed.
+ This has the effect of delaying the logging until right after the last
+ of the response is sent; ap_core_output_filter() calls the access logger
+ indirectly when it destroys the EOR bucket. [Brian Pane]
+
*) Rewrite of ap_core_output_filter to do nonblocking writes [Brian Pane]
*) Added new connection states for handler and write completion
Modified: httpd/httpd/branches/async-dev/include/http_request.h
URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/async-dev/include/http_request.h?rev=307339&r1=307338&r2=307339&view=diff
==============================================================================
--- httpd/httpd/branches/async-dev/include/http_request.h (original)
+++ httpd/httpd/branches/async-dev/include/http_request.h Sat Oct 8 18:37:11 2005
@@ -354,6 +354,36 @@
AP_DECLARE(int) ap_directory_walk(request_rec *r);
AP_DECLARE(int) ap_file_walk(request_rec *r);
+/** End Of REQUEST (EOR) bucket */
+AP_DECLARE_DATA extern const apr_bucket_type_t ap_bucket_type_eor;
+
+/**
+ * Determine if a bucket is an End Of REQUEST (EOR) bucket
+ * @param e The bucket to inspect
+ * @return true or false
+ */
+#define AP_BUCKET_IS_EOR(e) (e->type == &ap_bucket_type_eor)
+
+/**
+ * Make the bucket passed in an End Of REQUEST (EOR) bucket
+ * @param b The bucket to make into an EOR bucket
+ * @param r The request to destroy when this bucket is destroyed
+ * @return The new bucket, or NULL if allocation failed
+ */
+AP_DECLARE(apr_bucket *) ap_bucket_eor_make(apr_bucket *b, request_rec *r);
+
+/**
+ * Create a bucket referring to an End Of REQUEST (EOR). This bucket
+ * holds a pointer to the request_rec, so that the request can be
+ * destroyed right after all of the output has been sent to the client.
+ *
+ * @param list The freelist from which this bucket should be allocated
+ * @param r The request to destroy when this bucket is destroyed
+ * @return The new bucket, or NULL if allocation failed
+ */
+AP_DECLARE(apr_bucket *) ap_bucket_eor_create(apr_bucket_alloc_t *list,
+ request_rec *r);
+
#ifdef __cplusplus
}
#endif
Modified: httpd/httpd/branches/async-dev/modules/http/http_core.c
URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/async-dev/modules/http/http_core.c?rev=307339&r1=307338&r2=307339&view=diff
==============================================================================
--- httpd/httpd/branches/async-dev/modules/http/http_core.c (original)
+++ httpd/httpd/branches/async-dev/modules/http/http_core.c Sat Oct 8 18:37:11 2005
@@ -143,8 +143,6 @@
*/
cs->state = CONN_STATE_READ_REQUEST_LINE;
}
-
- apr_pool_destroy(r->pool);
}
else { /* ap_read_request failed - client may have closed */
cs->state = CONN_STATE_LINGER;
@@ -181,7 +179,6 @@
break;
ap_update_child_status(c->sbh, SERVER_BUSY_KEEPALIVE, r);
- apr_pool_destroy(r->pool);
if (ap_graceful_stop_signalled())
break;
Modified: httpd/httpd/branches/async-dev/modules/http/http_request.c
URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/async-dev/modules/http/http_request.c?rev=307339&r1=307338&r2=307339&view=diff
==============================================================================
--- httpd/httpd/branches/async-dev/modules/http/http_request.c (original)
+++ httpd/httpd/branches/async-dev/modules/http/http_request.c Sat Oct 8 18:37:11 2005
@@ -234,6 +234,8 @@
void ap_process_request(request_rec *r)
{
int access_status;
+ apr_bucket_brigade *bb;
+ apr_bucket *b;
/* Give quick handlers a shot at serving the request on the fast
* path, bypassing all of the other Apache hooks.
@@ -271,6 +273,15 @@
ap_die(access_status, r);
}
+ /* Send an EOR bucket through the output filter chain. When
+ * this bucket is destroyed, the request will be logged and
+ * its pool will be freed
+ */
+ bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
+ b = ap_bucket_eor_create(r->connection->bucket_alloc, r);
+ APR_BRIGADE_INSERT_HEAD(bb, b);
+ ap_pass_brigade(r->connection->output_filters, bb);
+
/*
* We want to flush the last packet if this isn't a pipelining connection
* *before* we start into logging. Suppose that the logging causes a DNS
@@ -280,7 +291,6 @@
*/
check_pipeline_flush(r);
ap_update_child_status(r->connection->sbh, SERVER_BUSY_LOG, r);
- ap_run_log_transaction(r);
if (ap_extended_status)
ap_time_process_request(r->connection->sbh, STOP_PREQUEST);
}
Modified: httpd/httpd/branches/async-dev/server/Makefile.in
URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/async-dev/server/Makefile.in?rev=307339&r1=307338&r2=307339&view=diff
==============================================================================
--- httpd/httpd/branches/async-dev/server/Makefile.in (original)
+++ httpd/httpd/branches/async-dev/server/Makefile.in Sat Oct 8 18:37:11 2005
@@ -14,7 +14,7 @@
mpm_common.c util_charset.c util_debug.c util_xml.c \
util_filter.c util_pcre.c exports.c \
scoreboard.c error_bucket.c protocol.c core.c request.c provider.c \
- eoc_bucket.c core_filters.c
+ eoc_bucket.c eor_bucket.c core_filters.c
TARGETS = delete-exports $(LTLIBRARY_NAME) $(CORE_IMPLIB_FILE) export_vars.h httpd.exp
Added: httpd/httpd/branches/async-dev/server/eor_bucket.c
URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/async-dev/server/eor_bucket.c?rev=307339&view=auto
==============================================================================
--- httpd/httpd/branches/async-dev/server/eor_bucket.c (added)
+++ httpd/httpd/branches/async-dev/server/eor_bucket.c Sat Oct 8 18:37:11 2005
@@ -0,0 +1,66 @@
+/* Copyright 2000-2005 The Apache Software Foundation or its licensors, as
+ * applicable.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "httpd.h"
+#include "http_request.h"
+
+static apr_status_t eor_bucket_read(apr_bucket *b, const char **str,
+ apr_size_t *len, apr_read_type_e block)
+{
+ *str = NULL;
+ *len = 0;
+ return APR_SUCCESS;
+}
+
+AP_DECLARE(apr_bucket *) ap_bucket_eor_make(apr_bucket *b, request_rec *r)
+{
+ b->length = 0;
+ b->start = 0;
+ b->data = r;
+ b->type = &ap_bucket_type_eor;
+
+ return b;
+}
+
+AP_DECLARE(apr_bucket *) ap_bucket_eor_create(apr_bucket_alloc_t *list,
+ request_rec *r)
+{
+ apr_bucket *b = apr_bucket_alloc(sizeof(*b), list);
+
+ APR_BUCKET_INIT(b);
+ b->free = apr_bucket_free;
+ b->list = list;
+ return ap_bucket_eor_make(b, r);
+}
+
+static void eor_bucket_destroy(void *data)
+{
+ request_rec *r = (request_rec *)data;
+ if (r != NULL) {
+ ap_run_log_transaction(r);
+ apr_pool_destroy(r->pool);
+ }
+}
+
+AP_DECLARE_DATA const apr_bucket_type_t ap_bucket_type_eor = {
+ "EOR", 5, APR_BUCKET_METADATA,
+ eor_bucket_destroy,
+ eor_bucket_read,
+ apr_bucket_setaside_noop,
+ apr_bucket_split_notimpl,
+ apr_bucket_simple_copy
+};
+
Re: svn commit: r307339 - in /httpd/httpd/branches/async-dev
Posted by Nick Kew <ni...@webthing.com>.
On Sunday 09 October 2005 20:48, Brian Pane wrote:
> If this example is a request-layer filter, no problem: the EOR
> buckets are inserted at the connection layer.
That's basically what I was hoping you'd confirm:-)
> . We'd
> just need to document the implied API rule:
Indeed. Better now than have two slightly-incompatible interpretations,
as we had with the "who owns a brigade" issue:-)
Problem closed, as far as I'm concerned - thanks:-)
--
Nick Kew
Re: svn commit: r307339 - in /httpd/httpd/branches/async-dev
Posted by Brian Pane <br...@apache.org>.
On Oct 9, 2005, at 11:41 AM, Nick Kew wrote:
> On Sunday 09 October 2005 18:49, Brian Pane wrote:
>
>> On Oct 9, 2005, at 3:25 AM, Nick Kew wrote:
>>
>>> On Sunday 09 October 2005 02:37, brianp@apache.org wrote:
>>>
>>>> URL: http://svn.apache.org/viewcvs?rev=307339&view=rev
>>>> Log:
>>>> Redesign of request cleanup:
>>>> - A new End-Of-Request bucket is pushed through the output filter
>>>> chain after the last bucket of the response.
>>>> - This bucket gets destroyed by ap_core_output_filter() after the
>>>> buckets in front of it have been sent.
>>>> - The destroy callback of the EOR bucket invokes the access
>>>> logger
>>>> and frees the request's pool.
>>>>
>>>
>>> How do you see this looking from a filter programmer's POV? I
>>> can see
>>> a danger of some nasty bugs arising from confusion between this and
>>> EOS buckets:
>>>
>>> - Existing filters test for EOS and will ignore EOR. That's
>>> presumably
>>> fine with you.
>>> - Sooner or later, someone will write a filter that propagates EOR
>>> and not EOS. Bang!
>>>
>>> Is there no way you could use EOS directly?
>>>
>>
>> I can think of two easy solutions:
>>
>> - Wrap the EOR declaration in "#ifdef CORE_PRIVATE," just
>> like we do for EOC.
>>
>
> So what does my filter see? A bucket of type <unknown>?
>
> How does this fit with filters of the common form:
>
> loop over buckets {
> if (is data) {
> read the data;
> write something to the next filter;
> } else if (is EOS) {
> write EOS to next filter;
> cleanup and return;
> }
> /* ignore anything else */
> }
>
> If there was an EOR bucket on input, it's lost in the above, yesno?
> How do you deal with that? If it doesn't break or break on this
> logic,
> I'm fine with it.
If this example is a request-layer filter, no problem: the EOR
buckets are inserted at the connection layer.
If a connection-layer filter uses this logic (throwing away
metadata buckets it doesn't understand), it's already broken,
since EOC buckets pass through the connection layer
output filters.
Unless I'm missing something, EOR buckets wouldn't
be at greater risk than EOC buckets are today. We'd
just need to document the implied API rule: "don't destroy
EOR buckets if you're not ap_core_output_filter." This
is a subtle enough API change that I wouldn't propose
backporting it to 2.0, but we could introduce it into 2.2
or 2.4.
>> - Or modify the EOS bucket API to support an optional "on_destroy"
>> callback.
>>
>
> Smells of side-effects. EOS buckets get created and destroyed by
> intermediate filters - as in my above skeleton.
I agree. There's a subset of problems that could be solved
by adding reference counts to the EOS buckets, but refcounts
wouldn't solve the general problem of request-layer filters that
destroy EOS filters and create new ones.
Brian
Re: svn commit: r307339 - in /httpd/httpd/branches/async-dev
Posted by Nick Kew <ni...@webthing.com>.
On Sunday 09 October 2005 18:49, Brian Pane wrote:
> On Oct 9, 2005, at 3:25 AM, Nick Kew wrote:
> > On Sunday 09 October 2005 02:37, brianp@apache.org wrote:
> >> URL: http://svn.apache.org/viewcvs?rev=307339&view=rev
> >> Log:
> >> Redesign of request cleanup:
> >> - A new End-Of-Request bucket is pushed through the output filter
> >> chain after the last bucket of the response.
> >> - This bucket gets destroyed by ap_core_output_filter() after the
> >> buckets in front of it have been sent.
> >> - The destroy callback of the EOR bucket invokes the access logger
> >> and frees the request's pool.
> >
> > How do you see this looking from a filter programmer's POV? I can see
> > a danger of some nasty bugs arising from confusion between this and
> > EOS buckets:
> >
> > - Existing filters test for EOS and will ignore EOR. That's
> > presumably
> > fine with you.
> > - Sooner or later, someone will write a filter that propagates EOR
> > and not EOS. Bang!
> >
> > Is there no way you could use EOS directly?
>
> I can think of two easy solutions:
>
> - Wrap the EOR declaration in "#ifdef CORE_PRIVATE," just
> like we do for EOC.
So what does my filter see? A bucket of type <unknown>?
How does this fit with filters of the common form:
loop over buckets {
if (is data) {
read the data;
write something to the next filter;
} else if (is EOS) {
write EOS to next filter;
cleanup and return;
}
/* ignore anything else */
}
If there was an EOR bucket on input, it's lost in the above, yesno?
How do you deal with that? If it doesn't break or break on this logic,
I'm fine with it.
> - Or modify the EOS bucket API to support an optional "on_destroy"
> callback.
Smells of side-effects. EOS buckets get created and destroyed by
intermediate filters - as in my above skeleton.
--
Nick Kew
Re: svn commit: r307339 - in /httpd/httpd/branches/async-dev
Posted by Brian Pane <br...@apache.org>.
On Oct 9, 2005, at 3:25 AM, Nick Kew wrote:
> On Sunday 09 October 2005 02:37, brianp@apache.org wrote:
>
>
>> URL: http://svn.apache.org/viewcvs?rev=307339&view=rev
>> Log:
>> Redesign of request cleanup:
>> - A new End-Of-Request bucket is pushed through the output filter
>> chain after the last bucket of the response.
>> - This bucket gets destroyed by ap_core_output_filter() after the
>> buckets in front of it have been sent.
>> - The destroy callback of the EOR bucket invokes the access logger
>> and frees the request's pool.
>>
>
> How do you see this looking from a filter programmer's POV? I can see
> a danger of some nasty bugs arising from confusion between this and
> EOS buckets:
>
> - Existing filters test for EOS and will ignore EOR. That's
> presumably
> fine with you.
> - Sooner or later, someone will write a filter that propagates EOR
> and not EOS. Bang!
>
> Is there no way you could use EOS directly?
I can think of two easy solutions:
- Wrap the EOR declaration in "#ifdef CORE_PRIVATE," just
like we do for EOC.
- Or modify the EOS bucket API to support an optional "on_destroy"
callback.
I slightly prefer the first option, but I'm open to suggestions.
Thanks,
Brian
Re: svn commit: r307339 - in /httpd/httpd/branches/async-dev
Posted by Nick Kew <ni...@webthing.com>.
On Sunday 09 October 2005 02:37, brianp@apache.org wrote:
> URL: http://svn.apache.org/viewcvs?rev=307339&view=rev
> Log:
> Redesign of request cleanup:
> - A new End-Of-Request bucket is pushed through the output filter
> chain after the last bucket of the response.
> - This bucket gets destroyed by ap_core_output_filter() after the
> buckets in front of it have been sent.
> - The destroy callback of the EOR bucket invokes the access logger
> and frees the request's pool.
How do you see this looking from a filter programmer's POV? I can see
a danger of some nasty bugs arising from confusion between this and
EOS buckets:
- Existing filters test for EOS and will ignore EOR. That's presumably
fine with you.
- Sooner or later, someone will write a filter that propagates EOR
and not EOS. Bang!
Is there no way you could use EOS directly?
--
Nick Kew