Re: Proxy regressions
On 11/10/2010 10:31 PM, Jim Jagielski wrote: > On Nov 10, 2010, at 9:25 AM, Graham Leggett wrote: > >> On 10 Nov 2010, at 4:13 PM, Plüm, Rüdiger, VF-Group wrote: >> >>> The core input filter in ap_core_input_filter which is used to read >>> the response from the backend creates the socket bucket from the conn rec >>> bucket allocator. So the used bucket allocator must live as long >>> as the conn rec of the backend connection lives. >>> And the conn rec of the backend connection lives longer then one request >>> and possibly longer than the frontend connection if the backend has >>> keepalive >>> enabled, in fact it is reused by the next frontend request that leases the >>> backend connection from the backend connection pool. >> In that case, the way to fix this would be to insert the frontend conn_rec's >> allocator into the backend conn_rec when we take it out of the pool, and >> then remove this allocator again when we're finished and place the >> connection back into the pool. >> >> This way, all our buckets would live as long as the frontend allocator, and >> we would be free to return the backend conn_rec back to the pool at any time. > > +1... Seems the only way to address this. > As long as underlying connection filters like mod_ssl do not buffer any buckets that need to used later once we get the connection back from the pool. I know I play devils advocate here :-). Regards Rüdiger
Re: svn commit: r1033779 - in /httpd/httpd/trunk: modules/cache/cache_common.h modules/cache/mod_cache.h modules/cache/mod_disk_cache.h support/htcacheclean.c
Am 11.11.2010 02:12, schrieb Graham Leggett: Being a little picky, but ideally mod_disk_cache should have it's own common header for htcacheclean to depend on, so that mod_disk_cache doesn't melt back into mod_cache. so you suggest the stuff extracted from mod_disk_cache.h should go into a separate disk_cache_common.h ? Gün.
Re: svn commit: r1033779 - in /httpd/httpd/trunk: modules/cache/cache_common.h modules/cache/mod_cache.h modules/cache/mod_disk_cache.h support/htcacheclean.c
On 11 Nov 2010, at 1:43 AM, fua...@apache.org wrote: Author: fuankg Date: Wed Nov 10 23:43:06 2010 New Revision: 1033779 URL: http://svn.apache.org/viewvc?rev=1033779&view=rev Log: Splitted off cache defines/structs used by htcacheclean. This makes htcacheclean again independent from httpd.h. Being a little picky, but ideally mod_disk_cache should have it's own common header for htcacheclean to depend on, so that mod_disk_cache doesn't melt back into mod_cache. Regards, Graham --
Re: svn commit: r1033779 - in /httpd/httpd/trunk: modules/cache/cache_common.h modules/cache/mod_cache.h modules/cache/mod_disk_cache.h support/htcacheclean.c
- Original Message -
From:
To:
Sent: Wednesday, November 10, 2010 5:43 PM
Subject: svn commit: r1033779 - in /httpd/httpd/trunk:
modules/cache/cache_common.h modules/cache/mod_cache.h
modules/cache/mod_disk_cache.h support/htcacheclean.c
Author: fuankg
Date: Wed Nov 10 23:43:06 2010
New Revision: 1033779
URL: http://svn.apache.org/viewvc?rev=1033779&view=rev
Log:
Splitted off cache defines/structs used by htcacheclean.
This makes htcacheclean again independent from httpd.h.
Submitted by: NormW
Added:
httpd/httpd/trunk/modules/cache/cache_common.h (with props)
Modified:
httpd/httpd/trunk/modules/cache/mod_cache.h
httpd/httpd/trunk/modules/cache/mod_disk_cache.h
httpd/httpd/trunk/support/htcacheclean.c
Added: httpd/httpd/trunk/modules/cache/cache_common.h
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/cache_common.h?rev=1033779&view=auto
==
--- httpd/httpd/trunk/modules/cache/cache_common.h (added)
+++ httpd/httpd/trunk/modules/cache/cache_common.h Wed Nov 10 23:43:06
2010
@@ -0,0 +1,94 @@
+/* Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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.
+ */
+
+/**
+ * @file cache_common.h
+ * @brief Common Cache vars/structs
+ *
+ * @defgroup Cache_cache Cache Functions
+ * @ingroup MOD_CACHE
+ * @{
+ */
+
+#ifndef CACHE_COMMON_H
+#define CACHE_COMMON_H
+
+#define VARY_FORMAT_VERSION 5
+#define DISK_FORMAT_VERSION 6
+
+#define CACHE_HEADER_SUFFIX ".header"
+#define CACHE_DATA_SUFFIX ".data"
+#define CACHE_VDIR_SUFFIX ".vary"
+
+#define AP_TEMPFILE_PREFIX "/"
+#define AP_TEMPFILE_BASE "aptmp"
+#define AP_TEMPFILE_SUFFIX "XX"
+#define AP_TEMPFILE_BASELEN strlen(AP_TEMPFILE_BASE)
+#define AP_TEMPFILE_NAMELEN strlen(AP_TEMPFILE_BASE AP_TEMPFILE_SUFFIX)
+#define AP_TEMPFILE AP_TEMPFILE_PREFIX AP_TEMPFILE_BASE
AP_TEMPFILE_SUFFIX
+
+/* a cache control header breakdown */
+typedef struct cache_control cache_control_t;
+struct cache_control {
+unsigned int parsed:1;
+unsigned int cache_control:1;
+unsigned int pragma:1;
+unsigned int no_cache:1;
+unsigned int no_cache_header:1; /* no cache by header match */
+unsigned int no_store:1;
+unsigned int max_age:1;
+unsigned int max_stale:1;
+unsigned int min_fresh:1;
+unsigned int no_transform:1;
+unsigned int only_if_cached:1;
+unsigned int public:1;
+unsigned int private:1;
+unsigned int private_header:1; /* private by header match */
+unsigned int must_revalidate:1;
+unsigned int proxy_revalidate:1;
+unsigned int s_maxage:1;
+apr_int64_t max_age_value; /* if positive, then set */
+apr_int64_t max_stale_value; /* if positive, then set */
+apr_int64_t min_fresh_value; /* if positive, then set */
+apr_int64_t s_maxage_value; /* if positive, then set */
+};
+
+
+typedef struct {
+/* Indicates the format of the header struct stored on-disk. */
+apr_uint32_t format;
+/* The HTTP status code returned for this response. */
+int status;
+/* The size of the entity name that follows. */
+apr_size_t name_len;
+/* The number of times we've cached this entity. */
+apr_size_t entity_version;
+/* Miscellaneous time values. */
+apr_time_t date;
+apr_time_t expire;
+apr_time_t request_time;
+apr_time_t response_time;
+/* The ident of the body file, so we can test the body matches the
header */
+apr_ino_t inode;
+apr_dev_t device;
+/* Does this cached request have a body? */
+int has_body:1;
+int header_only:1;
+/* The parsed cache control header */
+cache_control_t control;
+} disk_cache_info_t;
+
+#endif /* CACHE_COMMON_H */
Propchange: httpd/httpd/trunk/modules/cache/cache_common.h
--
svn:eol-style = native
Modified: httpd/httpd/trunk/modules/cache/mod_cache.h
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_cache.h?rev=1033779&r1=1033778&r2=1033779&view=diff
==
--- httpd/httpd/trunk/modules/cache/mod_cache.h (original)
+++ httpd/httpd/trunk/modules/cache/mo
Re: Proxy regressions
On Nov 10, 2010, at 9:25 AM, Graham Leggett wrote: > On 10 Nov 2010, at 4:13 PM, Plüm, Rüdiger, VF-Group wrote: > >> The core input filter in ap_core_input_filter which is used to read >> the response from the backend creates the socket bucket from the conn rec >> bucket allocator. So the used bucket allocator must live as long >> as the conn rec of the backend connection lives. >> And the conn rec of the backend connection lives longer then one request >> and possibly longer than the frontend connection if the backend has keepalive >> enabled, in fact it is reused by the next frontend request that leases the >> backend connection from the backend connection pool. > > In that case, the way to fix this would be to insert the frontend conn_rec's > allocator into the backend conn_rec when we take it out of the pool, and then > remove this allocator again when we're finished and place the connection back > into the pool. > > This way, all our buckets would live as long as the frontend allocator, and > we would be free to return the backend conn_rec back to the pool at any time. +1... Seems the only way to address this.
Re: SetVirtualDocumentRoot
On Wednesday 10 November 2010, Rich Bowen wrote: > On Nov 10, 2010, at 11:26 AM, Plüm, Rüdiger, VF-Group wrote: > >> -Original Message- > >> From: Rich Bowen > >> To: dev@httpd.apache.org > >> Subject: SetVirtualDocumentRoot > >> > >> I was reading bug reports this morning, and wondered if > >> someone could > >> take a look at the patch offered here: > >> > >> https://issues.apache.org/bugzilla/show_bug.cgi?id=26052#c19 > > > > The patch doesn't look appealing to me. Having a module changing > > the core config > > on the fly always looks nasty to me, even more so if it stores > > data in the core config > > structure that is allocated from a request pool. IMHO this is > > waiting for all > > nasty things to happen, SEGFAULT at best. > > Ok. Fair enough. How about something that set the ENV var for just > the current request? > > The frequency with which this gets asked seems to make it > worthwhile doing something, even if this patch isn't the right > thing to do. Maybe the larger solution of having a document_root field in the request_rec would be better. Has anyone had a chance to look at Ondrej's patch, already? I didn't get around to doing it, yet. https://issues.apache.org/bugzilla/show_bug.cgi?id=49705
Re: Any standard on picking a response status out of several possible?
On 10 Nov 2010, at 14:42, Dan Poirier wrote: > If multiple response statuses would be valid for a request (e.g. 403, > 413), is there any standard on how to pick one? I looked through RFC > 2616 but didn't see anything. Or is it just an implementation detail? This was subject of a fair bit of debate at the time; and from what I recall it was tried to reduce this to a non issue (i.e. letting the implementor make the call) by fairly carefully having the 2x/3x and 4x meaning right (success, retry with extra info and no-go) - along with making sure that the higher numbers are increasingly more narrow in their scope - with the assumption that implementors would only use those if the semantics where truly applicable and 'actionable' by a client. Guess that does not answer the question - but in your example 403 (forbidden) vs 413 (too large) - one probably wants to ask the question - is there anything the client can do (perhaps change some POST/form parameter to reduce the requested size) - or is it purely forbidden because if its size. If it is the first I'd do a 413 (so smarter clients can reduce what they get) and distinguish it form a 403 lost case - while if it is the latter - a 403 will be more meaningful to a wider range of clients. Hope that helps, Dw
RE: SetVirtualDocumentRoot
> -Original Message- > From: Rich Bowen > Sent: Mittwoch, 10. November 2010 17:35 > To: dev@httpd.apache.org > Subject: Re: SetVirtualDocumentRoot > > > On Nov 10, 2010, at 11:26 AM, Plüm, Rüdiger, VF-Group wrote: > > >> -Original Message- > >> From: Rich Bowen > >> To: dev@httpd.apache.org > >> Subject: SetVirtualDocumentRoot > >> > >> I was reading bug reports this morning, and wondered if > >> someone could > >> take a look at the patch offered here: > >> > >> https://issues.apache.org/bugzilla/show_bug.cgi?id=26052#c19 > > > > The patch doesn't look appealing to me. Having a module > changing the > > core config > > on the fly always looks nasty to me, even more so if it > stores data > > in the core config > > structure that is allocated from a request pool. IMHO this is > > waiting for all > > nasty things to happen, SEGFAULT at best. > > Ok. Fair enough. How about something that set the ENV var for > just the > current request? > > The frequency with which this gets asked seems to make it worthwhile > doing something, even if this patch isn't the right thing to do. My comment was only about the patch itself. I am yet undecided on the feature itself, but being +0 currently means I am in nobodys way :-). Regards Rüdiger
Re: SetVirtualDocumentRoot
On Nov 10, 2010, at 11:26 AM, Plüm, Rüdiger, VF-Group wrote: -Original Message- From: Rich Bowen To: dev@httpd.apache.org Subject: SetVirtualDocumentRoot I was reading bug reports this morning, and wondered if someone could take a look at the patch offered here: https://issues.apache.org/bugzilla/show_bug.cgi?id=26052#c19 The patch doesn't look appealing to me. Having a module changing the core config on the fly always looks nasty to me, even more so if it stores data in the core config structure that is allocated from a request pool. IMHO this is waiting for all nasty things to happen, SEGFAULT at best. Ok. Fair enough. How about something that set the ENV var for just the current request? The frequency with which this gets asked seems to make it worthwhile doing something, even if this patch isn't the right thing to do. -- Rich Bowen rbo...@rcbowen.com
RE: SetVirtualDocumentRoot
> -Original Message- > From: Rich Bowen > To: dev@httpd.apache.org > Subject: SetVirtualDocumentRoot > > I was reading bug reports this morning, and wondered if > someone could > take a look at the patch offered here: > > https://issues.apache.org/bugzilla/show_bug.cgi?id=26052#c19 The patch doesn't look appealing to me. Having a module changing the core config on the fly always looks nasty to me, even more so if it stores data in the core config structure that is allocated from a request pool. IMHO this is waiting for all nasty things to happen, SEGFAULT at best. Regards Rüdiger
Re: SetVirtualDocumentRoot
On Wed, Nov 10, 2010 at 10:35 AM, Rich Bowen wrote: > I was reading bug reports this morning, and wondered if someone could take a > look at the patch offered here: > > https://issues.apache.org/bugzilla/show_bug.cgi?id=26052#c19 > > It adds a SetVirtualDocumentRoot configuration directive to mod_vhost_alias, > making numerous third-party applications behave sanely when run under > mod_vhost_alias. > > I've read the various justifications why we don't do this, and quite frankly > I don't understand them. Is there a legitimate reason that setting > DOCUMENT_ROOT on a per-dynamic-vhost basis is any different/worse than > setting it on a per-regular-vhost basis? I don't understand the distinction, > other than "we've never done it that way." conceptually makes sense to me, at least in 2.3.x -- Eric Covener cove...@gmail.com
SetVirtualDocumentRoot
I was reading bug reports this morning, and wondered if someone could take a look at the patch offered here: https://issues.apache.org/bugzilla/show_bug.cgi?id=26052#c19 It adds a SetVirtualDocumentRoot configuration directive to mod_vhost_alias, making numerous third-party applications behave sanely when run under mod_vhost_alias. I've read the various justifications why we don't do this, and quite frankly I don't understand them. Is there a legitimate reason that setting DOCUMENT_ROOT on a per-dynamic-vhost basis is any different/ worse than setting it on a per-regular-vhost basis? I don't understand the distinction, other than "we've never done it that way." Thanks. -- Rich Bowen rbo...@rcbowen.com
RE: Any standard on picking a response status out of several possible?
I don't know of an existing standard order. What this proposed set of order: Send the highest number response code error (413 and not 403). For debug/testing environments, using expanded error messages or concatenated messages might be suitable. Richard -Original Message- From: Dan Poirier [mailto:poir...@pobox.com] Sent: Wednesday, November 10, 2010 9:43 AM To: dev@httpd.apache.org Subject: Any standard on picking a response status out of several possible? If multiple response statuses would be valid for a request (e.g. 403, 413), is there any standard on how to pick one? I looked through RFC 2616 but didn't see anything. Or is it just an implementation detail? Thanks, Dan
Any standard on picking a response status out of several possible?
If multiple response statuses would be valid for a request (e.g. 403, 413), is there any standard on how to pick one? I looked through RFC 2616 but didn't see anything. Or is it just an implementation detail? Thanks, Dan
Re: Proxy regressions
On 10 Nov 2010, at 3:54 PM, Plüm, Rüdiger, VF-Group wrote: As said this sounds doable for http backends, but not for https backends where we need to keep some data regarding the SSL state in the conn rec of the backend connection. This is entirely fine, it's only the contents of the buckets in the brigade that need to have a lifetime as long as the request, other SSL specific data can work as it does now, there is no need for it to be changed. A further issue is with backend servers where keepalive is switched off. Instead of acknowledging the connection close and releasing the connection, we hold the connection open for ages until the client finally acknowledges the request as finished. Is this a problem of a too long lingering close period on the backend server blocking the expensive backend server threads? I mean in general the backend server is the one who closes the connection if its keepalive timeout was used up and hence it can close the socket from its side. The only thing that comes to mind that could keep it blocked is a lingering close. Is this the case here? We do see this, yes, and it caused two outages in our testing environment recently caused by the initial version of the patch and the original pool lifetime bug. In v2.0, it was only saved in the connection if a keepalive was present. If there was no keepalive, it was released immediately. Which resulted in no connection pooling at all. Having a connection pool isn't a win if it comes at the cost of performance drag elsewhere. Ultimately the connections themselves in our case are far cheaper than the application servers behind them. Regards, Graham --
Re: Proxy regressions
On 10 Nov 2010, at 4:13 PM, Plüm, Rüdiger, VF-Group wrote: The core input filter in ap_core_input_filter which is used to read the response from the backend creates the socket bucket from the conn rec bucket allocator. So the used bucket allocator must live as long as the conn rec of the backend connection lives. And the conn rec of the backend connection lives longer then one request and possibly longer than the frontend connection if the backend has keepalive enabled, in fact it is reused by the next frontend request that leases the backend connection from the backend connection pool. In that case, the way to fix this would be to insert the frontend conn_rec's allocator into the backend conn_rec when we take it out of the pool, and then remove this allocator again when we're finished and place the connection back into the pool. This way, all our buckets would live as long as the frontend allocator, and we would be free to return the backend conn_rec back to the pool at any time. Regards, Graham --
Re: [RFC] Suexec On/Off directive
On Tue, Nov 9, 2010 at 2:18 PM, Stefan Fritsch wrote: >> Is there something special about the breakdown of suexec logic >> between mod_unixd and mod_suexec? Should this directive really be >> in mod_unixd since it owns the global suexec_enabled flag? > > Since it seems you don't need mod_suexec to use suexec (mod_userdir is > enough), I think it should be in mod_unixd. Alternatively, the setting > of the suexec_enabled flag should be moved to mod_suexec, too. funny, I have the same code in mod_unixd inside #if 0 (some effort on redistributing and/or consolidating various code in mod_unixd/unixd/mod_suexec would be worthwhile)
RE: Proxy regressions
> -Original Message- > From: "Plüm, Rüdiger, VF-Group" > Sent: Mittwoch, 10. November 2010 14:55 > To: dev@httpd.apache.org > Subject: RE: Proxy regressions > > > > > -Original Message- > > From: Graham Leggett > > Sent: Mittwoch, 10. November 2010 14:05 > > To: dev@httpd.apache.org > > Subject: Re: Proxy regressions > > > > On 10 Nov 2010, at 1:09 PM, Plüm, Rüdiger, VF-Group wrote: > > > > >> The proxy currently creates the allocator in > > >> ap_proxy_connection_create(), and then passes the > allocator to the > > >> various submodules via the ap_run_create_connection() hook, so it > > >> looks like we just passing the wrong allocator. > > > > > > The problem is that we keep the connrec structure in the > conn pool. > > > It is not created each time we fetch a connection from the > > conn pool. > > > This is required to enable keepalives with SSL backends. > > > As said if we pass the bucket allocator from the front end > > connection > > > we possibly end up with other pool lifetime issues and as > I speak of > > > it SSL comes to my mind. > > > > This doesn't sound right to me - ideally anything doing a read of > > anything that will ultimately be sent up the filter stack > should use > > the allocator belonging to the frontend connection. When > the backend > > connection is returned to the pool, the allocator should be > removed, > > and the next allocator inserted when the backend connection is > > subsequently reused. > > > > Currently what we seem to have is data allocated out of a > pool that > > has a lifetime completely unrelated to the frontend request, > > and then > > we're working around this by trying to keep this unrelated > > pool alive > > way longer than it's useful lifetime, and at least as long as the > > original request. This seems broken to me, we should really > be using > > the correct pools all the way through. > > As said this sounds doable for http backends, but not for > https backends > where we need to keep some data regarding the SSL state in > the conn rec > of the backend connection. The core input filter in ap_core_input_filter which is used to read the response from the backend creates the socket bucket from the conn rec bucket allocator. So the used bucket allocator must live as long as the conn rec of the backend connection lives. And the conn rec of the backend connection lives longer then one request and possibly longer than the frontend connection if the backend has keepalive enabled, in fact it is reused by the next frontend request that leases the backend connection from the backend connection pool. Regards Rüdiger
RE: Proxy regressions
> -Original Message- > From: Graham Leggett > Sent: Mittwoch, 10. November 2010 14:05 > To: dev@httpd.apache.org > Subject: Re: Proxy regressions > > On 10 Nov 2010, at 1:09 PM, Plüm, Rüdiger, VF-Group wrote: > > >> The proxy currently creates the allocator in > >> ap_proxy_connection_create(), and then passes the allocator to the > >> various submodules via the ap_run_create_connection() hook, so it > >> looks like we just passing the wrong allocator. > > > > The problem is that we keep the connrec structure in the conn pool. > > It is not created each time we fetch a connection from the > conn pool. > > This is required to enable keepalives with SSL backends. > > As said if we pass the bucket allocator from the front end > connection > > we possibly end up with other pool lifetime issues and as I speak of > > it SSL comes to my mind. > > This doesn't sound right to me - ideally anything doing a read of > anything that will ultimately be sent up the filter stack should use > the allocator belonging to the frontend connection. When the backend > connection is returned to the pool, the allocator should be removed, > and the next allocator inserted when the backend connection is > subsequently reused. > > Currently what we seem to have is data allocated out of a pool that > has a lifetime completely unrelated to the frontend request, > and then > we're working around this by trying to keep this unrelated > pool alive > way longer than it's useful lifetime, and at least as long as the > original request. This seems broken to me, we should really be using > the correct pools all the way through. As said this sounds doable for http backends, but not for https backends where we need to keep some data regarding the SSL state in the conn rec of the backend connection. > > >> Right now, we are holding backend connections open for as > long as it > >> takes for a frontend connection to acknowledge the request. A > >> typical > >> backend could be finished within milliseconds, while the > >> connection to > >> the frontend often takes hundreds, sometimes thousands of > >> milliseconds. While the backend connection is being held open, that > >> slot cannot be used by anyone else. > > > > Used by whom? > > Another worker in httpd. > > > As said if you put it back in the pool and your pool has the > > same max size as the number of threads in the process then > there is > > some > > chance that this connection will idle in the pool until the actual > > thread > > sent data to the client and fetches the connection from the pool > > again. > > As said I can only follow if the max pool size is configured to be > > smaller > > than the number of threads in the process. Do you do this? > > Yes. Threads in an application server are expensive, while > threads in > httpd are cheap. > > A further issue is with backend servers where keepalive is switched > off. Instead of acknowledging the connection close and releasing the > connection, we hold the connection open for ages until the client > finally acknowledges the request as finished. Is this a problem of a too long lingering close period on the backend server blocking the expensive backend server threads? I mean in general the backend server is the one who closes the connection if its keepalive timeout was used up and hence it can close the socket from its side. The only thing that comes to mind that could keep it blocked is a lingering close. Is this the case here? > > >> This issue is a regression that was introduced in httpd v2.2, httpd > >> 2.0 released the connection as soon as it was done. > > > > Because it had a completly different architecture and the released > > connection > > was not usable by anyone else but the same frontend connection > > because it was stored > > in the conn structure of the frontend request. So the result with > > 2.0 is the same > > as with 2.2. > > In v2.0, it was only saved in the connection if a keepalive was > present. If there was no keepalive, it was released immediately. Which resulted in no connection pooling at all. Regards Rüdiger
Re: Proxy regressions
On 10 Nov 2010, at 11:49 AM, Plüm, Rüdiger, VF-Group wrote: Have we not created a pool lifetime problem for ourselves here? In theory, any attempt to read from the backend connection should create buckets allocated from the r->connection->bucket_alloc allocator, which should be removed from the backend connection when the backend connection is returned to the pool. I guess we need a dedicated bucket_allocator at least in the beginning as we cannot guarantee that anyone in the create_connection hook uses the bucket_allocator to create an object that should persist until the connrec of the backend connection dies. Exchanging the allocator later each time we get the connection from the conn pool might create similar risks. But I admit that the later is only a gut feeling and I simply do not feel well with exchanging the allocator. I have no real hard facts why this cannot be done. The proxy currently creates the allocator in ap_proxy_connection_create(), and then passes the allocator to the various submodules via the ap_run_create_connection() hook, so it looks like we just passing the wrong allocator. So without trying to offend anyone, can we see the use case for the asap returning again? Right now, we are holding backend connections open for as long as it takes for a frontend connection to acknowledge the request. A typical backend could be finished within milliseconds, while the connection to the frontend often takes hundreds, sometimes thousands of milliseconds. While the backend connection is being held open, that slot cannot be used by anyone else. In addition, when backend keepalives are kept short (as ours are), the time it takes to serve a frontend request can exceed the keepalive timeout, creating unnecessary errors. This issue is a regression that was introduced in httpd v2.2, httpd 2.0 released the connection as soon as it was done. Regards, Graham --
Re: Proxy regressions
On 10 Nov 2010, at 1:09 PM, Plüm, Rüdiger, VF-Group wrote: The proxy currently creates the allocator in ap_proxy_connection_create(), and then passes the allocator to the various submodules via the ap_run_create_connection() hook, so it looks like we just passing the wrong allocator. The problem is that we keep the connrec structure in the conn pool. It is not created each time we fetch a connection from the conn pool. This is required to enable keepalives with SSL backends. As said if we pass the bucket allocator from the front end connection we possibly end up with other pool lifetime issues and as I speak of it SSL comes to my mind. This doesn't sound right to me - ideally anything doing a read of anything that will ultimately be sent up the filter stack should use the allocator belonging to the frontend connection. When the backend connection is returned to the pool, the allocator should be removed, and the next allocator inserted when the backend connection is subsequently reused. Currently what we seem to have is data allocated out of a pool that has a lifetime completely unrelated to the frontend request, and then we're working around this by trying to keep this unrelated pool alive way longer than it's useful lifetime, and at least as long as the original request. This seems broken to me, we should really be using the correct pools all the way through. Right now, we are holding backend connections open for as long as it takes for a frontend connection to acknowledge the request. A typical backend could be finished within milliseconds, while the connection to the frontend often takes hundreds, sometimes thousands of milliseconds. While the backend connection is being held open, that slot cannot be used by anyone else. Used by whom? Another worker in httpd. As said if you put it back in the pool and your pool has the same max size as the number of threads in the process then there is some chance that this connection will idle in the pool until the actual thread sent data to the client and fetches the connection from the pool again. As said I can only follow if the max pool size is configured to be smaller than the number of threads in the process. Do you do this? Yes. Threads in an application server are expensive, while threads in httpd are cheap. A further issue is with backend servers where keepalive is switched off. Instead of acknowledging the connection close and releasing the connection, we hold the connection open for ages until the client finally acknowledges the request as finished. This issue is a regression that was introduced in httpd v2.2, httpd 2.0 released the connection as soon as it was done. Because it had a completly different architecture and the released connection was not usable by anyone else but the same frontend connection because it was stored in the conn structure of the frontend request. So the result with 2.0 is the same as with 2.2. In v2.0, it was only saved in the connection if a keepalive was present. If there was no keepalive, it was released immediately. Regards, Graham --
RE: svn commit: r1033145 - /httpd/httpd/trunk/modules/metadata/mod_setenvif.c
> -Original Message-
> From: Stefan Fritsch
> Sent: Mittwoch, 10. November 2010 11:56
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1033145 -
> /httpd/httpd/trunk/modules/metadata/mod_setenvif.c
>
> On Wed, 10 Nov 2010, Ruediger Pluem wrote:
> > On 11/09/2010 07:34 PM, s...@apache.org wrote:
> >> Author: sf
> >> Date: Tue Nov 9 18:34:43 2010
> >> New Revision: 1033145
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1033145&view=rev
> >> Log:
> >> use temp_pool for some temporary regexps
> >>
> >> Modified:
> >> httpd/httpd/trunk/modules/metadata/mod_setenvif.c
> >>
> >> Modified: httpd/httpd/trunk/modules/metadata/mod_setenvif.c
> >> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/metadat
> a/mod_setenvif.c?rev=1033145&r1=1033144&r2=1033145&view=diff
> >>
> ==
>
> >> --- httpd/httpd/trunk/modules/metadata/mod_setenvif.c (original)
> >> +++ httpd/httpd/trunk/modules/metadata/mod_setenvif.c Tue
> Nov 9 18:34:43 2010
> >> @@ -355,7 +355,7 @@ static const char *add_setenvif_core(cmd
> >> new->special_type = SPECIAL_OID_VALUE;
> >>
> >> /* Syntax check and extraction of the OID as
> a regex: */
> >> -new->pnamereg = ap_pregcomp(cmd->pool,
> >> +new->pnamereg = ap_pregcomp(cmd->temp_pool,
> >>
> "^oid\\(\"?([0-9.]+)\"?\\)$",
> >> (AP_REG_EXTENDED
> /* | AP_REG_NOSUB */
> >> | AP_REG_ICASE));
> >> @@ -381,7 +381,7 @@ static const char *add_setenvif_core(cmd
> >> * (new->pnamereg = NULL) to avoid the
> overhead of searching
> >> * through headers_in for a regex match.
> >> */
> >> -if (is_header_regex(cmd->pool, fname)) {
> >> +if (is_header_regex(cmd->temp_pool, fname)) {
> >
> > Are you sure this is correct here? I don't see new->pnamereg being
> > discarded afterwards (like in above context).
>
> I kept cmd->pool for this regexp. But is_header_regex internally also
> creates a temp regex that it does not store anywhere.
Ahh. My bad. I got in the wrong line.
Thanks for pointing out and explaining.
Regards
Rüdiger
RE: Proxy regressions
> -Original Message- > From: Graham Leggett > Sent: Mittwoch, 10. November 2010 11:47 > To: dev@httpd.apache.org > Subject: Re: Proxy regressions > > On 10 Nov 2010, at 11:49 AM, Plüm, Rüdiger, VF-Group wrote: > > >> Have we not created a pool lifetime problem for ourselves here? > >> > >> In theory, any attempt to read from the backend connection should > >> create buckets allocated from the r->connection->bucket_alloc > >> allocator, which should be removed from the backend connection when > >> the backend connection is returned to the pool. > > > > I guess we need a dedicated bucket_allocator at least in > the beginning > > as we cannot guarantee that anyone in the create_connection > hook uses > > the bucket_allocator to create an object that should > persist until the > > connrec of the backend connection dies. > > > > Exchanging the allocator later each time we get the connection from > > the conn pool might create similar risks. But I admit that the later > > is only a gut feeling and I simply do not feel well with > exchanging > > the > > allocator. I have no real hard facts why this cannot be done. > > The proxy currently creates the allocator in > ap_proxy_connection_create(), and then passes the allocator to the > various submodules via the ap_run_create_connection() hook, so it > looks like we just passing the wrong allocator. The problem is that we keep the connrec structure in the conn pool. It is not created each time we fetch a connection from the conn pool. This is required to enable keepalives with SSL backends. As said if we pass the bucket allocator from the front end connection we possibly end up with other pool lifetime issues and as I speak of it SSL comes to my mind. > > > So without trying to offend anyone, can we see the use case > for the > > asap returning > > again? > > Right now, we are holding backend connections open for as long as it > takes for a frontend connection to acknowledge the request. A > typical > backend could be finished within milliseconds, while the > connection to > the frontend often takes hundreds, sometimes thousands of > milliseconds. While the backend connection is being held open, that > slot cannot be used by anyone else. Used by whom? As said if you put it back in the pool and your pool has the same max size as the number of threads in the process then there is some chance that this connection will idle in the pool until the actual thread sent data to the client and fetches the connection from the pool again. As said I can only follow if the max pool size is configured to be smaller than the number of threads in the process. Do you do this? Another possibility would be that depending on the request behaviour on your frontend and the distribution between locally handled requests (e.g. static content, cache) and backend content it might happen that the number of actual backend connections in the pool does not increase that much (aka. to its max size) if the connection is returned to the pool asap. Do you intend to get this effect? > > In addition, when backend keepalives are kept short (as ours > are), the > time it takes to serve a frontend request can exceed the keepalive > timeout, creating unnecessary errors. Why does this create errors? The connection is released by the backend because it has delivered all data to the frontend server and has not received a new request within the keepalive timeframe. So the backend is actually free to reuse these resources. And the frontend will notice that the backend has disconnected the next time it fetches the connection again from the pool and will establish a new connection. > > This issue is a regression that was introduced in httpd v2.2, httpd > 2.0 released the connection as soon as it was done. Because it had a completly different architecture and the released connection was not usable by anyone else but the same frontend connection because it was stored in the conn structure of the frontend request. So the result with 2.0 is the same as with 2.2. Regards Rüdiger
Re: Proxy regressions
On 10 Nov 2010, at 9:02 AM, Ruediger Pluem wrote: The fix in r1030855 is wrong: ap_proxy_buckets_lifetime_transform is not copying the data but only creates transient buckets from the data in the buckets in bb. If you then destroy bb before passing pass_bb, the data where the buckets in pass_bb point gets freed and later gets overwritten. Good catch. I reviewed the code again and then remembered the idea why we use transient buckets: If everything goes well and the data is sent out by ap_pass_brigade no copying of the contents of the buckets is needed. Only if things are buffered somewhere down the chain, the according filter needs to set the buckets aside (which causes copying). So I guess with the approach to release the backend connection immediately we will loose this advantage. That is regrettable. I guess an easy solution would be to use heap buckets instead of transient buckets. Have we not created a pool lifetime problem for ourselves here? In theory, any attempt to read from the backend connection should create buckets allocated from the r->connection->bucket_alloc allocator, which should be removed from the backend connection when the backend connection is returned to the pool. Instead, it seems we create our own bucket_alloc from the backend connection's pool, and then we only work because we're holding onto the backend until we've stopped writing buckets - far longer than we should be, and all along we've been working, but only by accident. Regards, Graham --
Re: svn commit: r1033145 - /httpd/httpd/trunk/modules/metadata/mod_setenvif.c
On Wed, 10 Nov 2010, Ruediger Pluem wrote:
On 11/09/2010 07:34 PM, s...@apache.org wrote:
Author: sf
Date: Tue Nov 9 18:34:43 2010
New Revision: 1033145
URL: http://svn.apache.org/viewvc?rev=1033145&view=rev
Log:
use temp_pool for some temporary regexps
Modified:
httpd/httpd/trunk/modules/metadata/mod_setenvif.c
Modified: httpd/httpd/trunk/modules/metadata/mod_setenvif.c
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/metadata/mod_setenvif.c?rev=1033145&r1=1033144&r2=1033145&view=diff
==
--- httpd/httpd/trunk/modules/metadata/mod_setenvif.c (original)
+++ httpd/httpd/trunk/modules/metadata/mod_setenvif.c Tue Nov 9 18:34:43 2010
@@ -355,7 +355,7 @@ static const char *add_setenvif_core(cmd
new->special_type = SPECIAL_OID_VALUE;
/* Syntax check and extraction of the OID as a regex: */
-new->pnamereg = ap_pregcomp(cmd->pool,
+new->pnamereg = ap_pregcomp(cmd->temp_pool,
"^oid\\(\"?([0-9.]+)\"?\\)$",
(AP_REG_EXTENDED /* | AP_REG_NOSUB */
| AP_REG_ICASE));
@@ -381,7 +381,7 @@ static const char *add_setenvif_core(cmd
* (new->pnamereg = NULL) to avoid the overhead of searching
* through headers_in for a regex match.
*/
-if (is_header_regex(cmd->pool, fname)) {
+if (is_header_regex(cmd->temp_pool, fname)) {
Are you sure this is correct here? I don't see new->pnamereg being
discarded afterwards (like in above context).
I kept cmd->pool for this regexp. But is_header_regex internally also
creates a temp regex that it does not store anywhere.
new->pnamereg = ap_pregcomp(cmd->pool, fname,
(AP_REG_EXTENDED | AP_REG_NOSUB
| (icase ? AP_REG_ICASE : 0)));
RE: Proxy regressions
> -Original Message- > From: Graham Leggett [mailto:minf...@sharp.fm] > Sent: Mittwoch, 10. November 2010 10:28 > To: dev@httpd.apache.org > Subject: Re: Proxy regressions > > On 10 Nov 2010, at 9:02 AM, Ruediger Pluem wrote: > > >> The fix in r1030855 is wrong: > ap_proxy_buckets_lifetime_transform is > >> not copying the data but only creates transient buckets > from the data > >> in the buckets in bb. If you then destroy bb before > passing pass_bb, > >> the data where the buckets in pass_bb point gets freed and > later gets > >> overwritten. > > > > Good catch. I reviewed the code again and then remembered > the idea why > > we use transient buckets: If everything goes well and the data is > > sent out > > by ap_pass_brigade no copying of the contents of the buckets is > > needed. > > Only if things are buffered somewhere down the chain, the > according > > filter > > needs to set the buckets aside (which causes copying). > > So I guess with the approach to release the backend connection > > immediately > > we will loose this advantage. That is regrettable. > > I guess an easy solution would be to use heap buckets instead of > > transient > > buckets. > > Have we not created a pool lifetime problem for ourselves here? > > In theory, any attempt to read from the backend connection should > create buckets allocated from the r->connection->bucket_alloc > allocator, which should be removed from the backend connection when > the backend connection is returned to the pool. I guess we need a dedicated bucket_allocator at least in the beginning as we cannot guarantee that anyone in the create_connection hook uses the bucket_allocator to create an object that should persist until the connrec of the backend connection dies. Exchanging the allocator later each time we get the connection from the conn pool might create similar risks. But I admit that the later is only a gut feeling and I simply do not feel well with exchanging the allocator. I have no real hard facts why this cannot be done. > > Instead, it seems we create our own bucket_alloc from the backend > connection's pool, and then we only work because we're holding onto > the backend until we've stopped writing buckets - far longer than we > should be, and all along we've been working, but only by accident. In the light of these regressions I took a step back. My first thought on returning the backend connection to the connection pool asap was: This makes sense. Now after taking a step back I ask myself: Why is this benefitial? In the default configuration your connection pool to the backend can be as large as the number of threads you use per process in your MPM. So not returning it to the conn pool asap does not block any other thread from getting a connection to the backend from this conn pool. Only one exception comes to my mind where the asap returning makes sense: The maxium pool size has been configured to be lower than the number of threads. So without trying to offend anyone, can we see the use case for the asap returning again? Regards Rüdiger
