Re: svn commit: r711886 - /httpd/httpd/branches/2.2.x/STATUS
On 11/07/2008 05:33 PM, Joe Orton wrote:
On Fri, Nov 07, 2008 at 01:29:15PM +0100, Plüm, Rüdiger, VF-Group wrote:
Would it be possible to substitute the backend (fake) conn_rec's
-bucket_alloc pointer with the real r-connection-bucket_alloc,
for the duration of the request/response to the backend? Wouldn't
that ensure that all the buckets in question exactly have the
correct lifetime?
I though about this as well, but I suppose this is a risky road to take.
Keep in mind that mod_ssl and the core network filters below might use this
bucket allocator and that we keep this backend connection alive while the
front
end connection might have died. So we might still have buckets in this
backend connection pipe created by the previous request that are dead on the
new request. This may cause segfaults when we start using the backend
connection again.
Fair enough. Transmuting the buckets into TRANSIENTs seems like a
reasonable solution, in any case, it's sort of saying lifetime issues,
buyer beware in bucket-speak which exactly matches the problem in hand.
In ap_proxy_buckets_lifetime_transform() is there any reason to restrict
to:
+if (APR_BUCKET_IS_HEAP(e) || APR_BUCKET_IS_TRANSIENT(e)
+|| APR_BUCKET_IS_IMMORTAL(e) || APR_BUCKET_IS_POOL(e)) {
+apr_bucket_read(e, data, bytes, APR_BLOCK_READ);
instead of simply:
if (!APR_BUCKET_IS_METADATA(e)) {
// ...likewise transmute to a TRANSIENT
? You already know that the brigade size is constrained, right, so
there is no worry about memory consumption from morphing buckets, and
the technique should should work equally well for any data bucket?
This is a valid and good point I have not though about before :-).
Especially that the length of the brigade is limited. And it seems better
in this case to possibly do a sub optimal conversion of a data bucket
(like an MMAP bucket or especially a file bucket) than to not convert it at
all. So I adjusted this part of the code. Thanks for reviewing.
Regards
Rüdiger
Re: svn commit: r711886 - /httpd/httpd/branches/2.2.x/STATUS
Joe Orton wrote: What is the problem at all? mod_proxy_http uses a a conn_rec to communicate with the backend. It somehow reverses the meaning of input and output filters and uses them to send the request and receive the response. In order to use persistent SSL connections to the backend it is needed to keep this conn_rec (and thus its bucket allocator) alive and tied to the backend connection. Thus it is no longer destroyed at the end of a client request / connection. So the conn_rec and especially the bucket allocator for the backend have a lifecycle that is disjoint from the one to the client. Especially it can happen that due to e.g. a faulty backend connection the conn_rec to the backend *lives* shorter than buckets that are still buffered somewhere in the output filter chain to the client (see comments in removed code below). This causes segfaults when these buckets are accessed then or if brigades that contain them get cleaned up later on due to a parent pool cleanup. Not sure if I've got my head round this correctly, but... Would it be possible to substitute the backend (fake) conn_rec's -bucket_alloc pointer with the real r-connection-bucket_alloc, for the duration of the request/response to the backend? Wouldn't that ensure that all the buckets in question exactly have the correct lifetime? What looks broken to me is the fact that there is a link between the backend connection_rec and the frontend request_rec at all. Both of these structures have completely independent and unrelated lifecycles, and either structure might outlive the other structure. Ideally there needs to be a way of passing data that belongs to the backend across to the frontend without making any assumption that one will outlive the other. If this isn't possible, then we'll just be forced to copy the data. Regards, Graham -- smime.p7s Description: S/MIME Cryptographic Signature
Re: svn commit: r711886 - /httpd/httpd/branches/2.2.x/STATUS
On Thu, Nov 06, 2008 at 09:58:52PM +0100, Ruediger Pluem wrote: What is the problem at all? mod_proxy_http uses a a conn_rec to communicate with the backend. It somehow reverses the meaning of input and output filters and uses them to send the request and receive the response. In order to use persistent SSL connections to the backend it is needed to keep this conn_rec (and thus its bucket allocator) alive and tied to the backend connection. Thus it is no longer destroyed at the end of a client request / connection. So the conn_rec and especially the bucket allocator for the backend have a lifecycle that is disjoint from the one to the client. Especially it can happen that due to e.g. a faulty backend connection the conn_rec to the backend *lives* shorter than buckets that are still buffered somewhere in the output filter chain to the client (see comments in removed code below). This causes segfaults when these buckets are accessed then or if brigades that contain them get cleaned up later on due to a parent pool cleanup. Not sure if I've got my head round this correctly, but... Would it be possible to substitute the backend (fake) conn_rec's -bucket_alloc pointer with the real r-connection-bucket_alloc, for the duration of the request/response to the backend? Wouldn't that ensure that all the buckets in question exactly have the correct lifetime? Regards, Joe
Re: svn commit: r711886 - /httpd/httpd/branches/2.2.x/STATUS
On Fri, Nov 07, 2008 at 01:29:15PM +0100, Plüm, Rüdiger, VF-Group wrote:
Would it be possible to substitute the backend (fake) conn_rec's
-bucket_alloc pointer with the real r-connection-bucket_alloc,
for the duration of the request/response to the backend? Wouldn't
that ensure that all the buckets in question exactly have the
correct lifetime?
I though about this as well, but I suppose this is a risky road to take.
Keep in mind that mod_ssl and the core network filters below might use this
bucket allocator and that we keep this backend connection alive while the
front
end connection might have died. So we might still have buckets in this
backend connection pipe created by the previous request that are dead on the
new request. This may cause segfaults when we start using the backend
connection again.
Fair enough. Transmuting the buckets into TRANSIENTs seems like a
reasonable solution, in any case, it's sort of saying lifetime issues,
buyer beware in bucket-speak which exactly matches the problem in hand.
In ap_proxy_buckets_lifetime_transform() is there any reason to restrict
to:
+if (APR_BUCKET_IS_HEAP(e) || APR_BUCKET_IS_TRANSIENT(e)
+|| APR_BUCKET_IS_IMMORTAL(e) || APR_BUCKET_IS_POOL(e)) {
+apr_bucket_read(e, data, bytes, APR_BLOCK_READ);
instead of simply:
if (!APR_BUCKET_IS_METADATA(e)) {
// ...likewise transmute to a TRANSIENT
? You already know that the brigade size is constrained, right, so
there is no worry about memory consumption from morphing buckets, and
the technique should should work equally well for any data bucket?
Apologies if you've already thought about this too but it might be worth
a comment in the code explaining why, if so :)
Regards, Joe
Re: svn commit: r711886 - /httpd/httpd/branches/2.2.x/STATUS
-Ursprüngliche Nachricht- Von: Joe Orton [mailto:[EMAIL PROTECTED] Gesendet: Freitag, 7. November 2008 13:11 An: dev@httpd.apache.org Betreff: Re: svn commit: r711886 - /httpd/httpd/branches/2.2.x/STATUS On Thu, Nov 06, 2008 at 09:58:52PM +0100, Ruediger Pluem wrote: What is the problem at all? mod_proxy_http uses a a conn_rec to communicate with the backend. It somehow reverses the meaning of input and output filters and uses them to send the request and receive the response. In order to use persistent SSL connections to the backend it is needed to keep this conn_rec (and thus its bucket allocator) alive and tied to the backend connection. Thus it is no longer destroyed at the end of a client request / connection. So the conn_rec and especially the bucket allocator for the backend have a lifecycle that is disjoint from the one to the client. Especially it can happen that due to e.g. a faulty backend connection the conn_rec to the backend *lives* shorter than buckets that are still buffered somewhere in the output filter chain to the client (see comments in removed code below). This causes segfaults when these buckets are accessed then or if brigades that contain them get cleaned up later on due to a parent pool cleanup. Not sure if I've got my head round this correctly, but... Would it be possible to substitute the backend (fake) conn_rec's -bucket_alloc pointer with the real r-connection-bucket_alloc, for the duration of the request/response to the backend? Wouldn't that ensure that all the buckets in question exactly have the correct lifetime? I though about this as well, but I suppose this is a risky road to take. Keep in mind that mod_ssl and the core network filters below might use this bucket allocator and that we keep this backend connection alive while the front end connection might have died. So we might still have buckets in this backend connection pipe created by the previous request that are dead on the new request. This may cause segfaults when we start using the backend connection again. Regards Rüdiger
Re: svn commit: r711886 - /httpd/httpd/branches/2.2.x/STATUS
On 11/06/2008 05:07 PM, [EMAIL PROTECTED] wrote:
Author: jim
Date: Thu Nov 6 08:06:54 2008
New Revision: 711886
URL: http://svn.apache.org/viewvc?rev=711886view=rev
Log:
Hold off until I learn more from Ruediger
Modified:
httpd/httpd/branches/2.2.x/STATUS
Modified: httpd/httpd/branches/2.2.x/STATUS
URL:
http://svn.apache.org/viewvc/httpd/httpd/branches/2.2.x/STATUS?rev=711886r1=711885r2=711886view=diff
==
--- httpd/httpd/branches/2.2.x/STATUS (original)
+++ httpd/httpd/branches/2.2.x/STATUS Thu Nov 6 08:06:54 2008
@@ -135,7 +135,7 @@
http://svn.apache.org/viewvc?rev=706921view=rev
Backport version for 2.2.x of patch:
Trunk version of patch works
- +1: jim
+ +1:
rpluem: This code still has bad edge cases that lead to segfaults. I
have
a better version in the pipe that currently undergoes testing and will
be in trunk soon.
After analysing some crashes on one of my systems I came to the conclusion that
the
current approach to solve the problem still has open edge cases of which some
IMHO
could not be fixed this way. Furthermore the already complex flushing code
would become
even more complex and harder to understand. So I chose a new approach to solve
the problem.
What is the problem at all?
mod_proxy_http uses a a conn_rec to communicate with the backend. It somehow
reverses
the meaning of input and output filters and uses them to send the request and
receive
the response. In order to use persistent SSL connections to the backend it is
needed
to keep this conn_rec (and thus its bucket allocator) alive and tied to the
backend
connection. Thus it is no longer destroyed at the end of a client request /
connection.
So the conn_rec and especially the bucket allocator for the backend have a
lifecycle
that is disjoint from the one to the client. Especially it can happen that due
to e.g.
a faulty backend connection the conn_rec to the backend *lives* shorter than
buckets that are still buffered somewhere in the output filter chain to the
client
(see comments in removed code below). This causes segfaults when these buckets
are
accessed then or if brigades that contain them get cleaned up later on due to a
parent
pool cleanup.
The new approach to solve the problem is to transform the lifetime of the
buckets
by shifting them to a different allocator. The basic approach is to iterate over
the brigade fetched from the backend connection and
for metabuckets recreate them
for data buckets read them and put the read data in a transient bucket.
Currently only HEAP, TRANSIENT, POOL and IMMORTAL data buckets are expected
from the backend.
In this case apr_bucket_read is a trivial operation that only returns a pointer
to the
data stored in the bucket. Once we pass the buckets down the chain filters are
responsible
to set the buckets aside if they do not consume them in this filter pass.
Setting aside
a transient bucket causes the data to be moved into a freshly allocated memory
area that
is allocated from the bucket allocator, in this case the correct one as it is
the one from
the connection to the client. This way effort intensive memory copying only
happens, when
really needed. A big plus of this approach is that it opens the door again for
returning
the backend connection back to the connection pool as soon as we have received
all data
from the backend instead of the need to wait until all data is flushed to the
client.
I guess this will make some people happy here :-).
Well here we go:
Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c (revision 711224)
+++ modules/proxy/proxy_util.c (working copy)
@@ -1628,9 +1628,6 @@
{
proxy_conn_rec *conn = (proxy_conn_rec *)theconn;
proxy_worker *worker = conn-worker;
-apr_bucket_brigade *bb;
-conn_rec *c;
-request_rec *r;
/*
* If the connection pool is NULL the worker
@@ -1651,112 +1648,6 @@
}
#endif
-r = conn-r;
-if (conn-need_flush r) {
-request_rec *main_request;
-
-/*
- * We need to get to the main request as subrequests do not count any
- * sent bytes.
- */
-main_request = r;
-while (main_request-main) {
-main_request = main_request-main;
-}
-if (main_request-bytes_sent || r-eos_sent) {
-ap_filter_t *flush_filter;
-
-/*
- * We need to ensure that buckets that may have been buffered in
the
- * network filters get flushed to the network. This is needed since
- * these buckets have been created with the bucket allocator of the
- * backend connection. This allocator either gets destroyed if
- * conn-close is set or the worker address is not reusable which
- * causes the
