Re: cvs commit: httpd-2.0/server core.c request.c
brianp 01/12/31 00:18:32
Modified:include http_core.h
server core.c request.c
Log:
Performance fix for prep_walk_cache():
Moved the directory/location/file-walk caches from the
request's pool userdata hash table to the core_request_config
struct.
This change removes about 60% of the processing time from
prep_walk_cache().
Brian... this is a very cool patch...
+typedef enum {
+AP_WALK_DIRECTORY,
+AP_WALK_LOCATION,
+AP_WALK_FILE,
+AP_NUM_WALK_CACHES
+} ap_walk_cache_type;
+
typedef struct {
/* bucket brigade used by getline for look-ahead and
* ap_get_client_block for holding left-over request body */
struct apr_bucket_brigade *bb;
+
+/* a place to hold per-request working data for
+ * ap_directory_walk, ap_location_walk, and ap_file_walk */
+void *walk_cache[AP_NUM_WALK_CACHES];
} core_request_config;
However, can we find a way to make AP_NUM_WALK_CACHES a bit more flexible?
See, for example, mod_proxy. We are doing the same bit there. Would you
mind extending your patch to allow registration [at startup] of other
cache members? This could be used for walk caches, or for other sorts of
per-request cached data that currently resides in notes (and should reside
in userdata, until you pointed out how slow that solution really is.)
In any case, nice work :)
Bill
Re: cvs commit: httpd-2.0/server core.c request.c
William A. Rowe, Jr. wrote: ... However, can we find a way to make AP_NUM_WALK_CACHES a bit more flexible? See, for example, mod_proxy. We are doing the same bit there. Would you mind extending your patch to allow registration [at startup] of other cache members? This could be used for walk caches, or for other sorts of per-request cached data that currently resides in notes (and should reside in userdata, until you pointed out how slow that solution really is.) I like the registration idea. I'll work out an API for it sometime this week. --Brian
Input Filtering and Pushback.
I've attempted to think some more about how we should handle pushback in the input filtering (say that a caller does a speculative read - i.e. MIME-continuation in ap_[r]getline). I think the best strategy would be to introduce a new input filter mode (call it PEEK and rename the current PEEK mode to EATCRLF) that will indicate to input filters that this data may not be consumed and that it should hold on to the data if possible so that it can be read again (on a non-PEEK call). Using this strategy wisely should remove all instances of a req_cfg-bb (such as what ap_[r]getline uses). Data may only be stored inside the filter chain (no external brigades with unread input are allowed!). What follows is my high-level algorithm on how ap_[r]getline would work with this strategy. Please provide any feedback or thoughts you may have on this. Thanks. -- justin ap_getline with buckets and brigades: 1. call ap_get_brigade with 0 to indicate want LF-line. - ap_get_brigade may return with a line that does not have a LF-ending (i.e. len == HUGE_STRING_LEN). 2. Check to see if we got an error value back (!= APR_SUCCESS) 3. Call apr_brigade_length to determine length of brigade. 4. Do we have enough space in our caller's buffer to handle this? (If we're not doing the allocation for them, that is.) 5. Run through all buckets in the brigade and read/copy into the caller's buffer. (What happened to zero-copy?) 6. Does last bucket's data (APR_BRIGADE_LAST) have LF char? - If no LF, go to step 1. 7. We now have a LF-line (otherwise, we'd error out). 8. Attempt to compress the EOL by removing any CR or whitespace. - This would maintain backwards compatibility, but may no longer be worthwhile. 9. Are we asked to handling folding? - If not, then translate the buffer from ascii and return. 10. Call ap_get_brigade with PEEK. - Should this PEEK call block? Is that an option that the caller to ap_get_brigade specifies? I think ap_getline would indeed want it to be a blocking PEEK. - This peek is not the same as current AP_MODE_PEEK (which just eats CRLFs). It would require some modifications to the current input filters to ensure that each filter does *NOT* delete the returned data from its input stack, but instead copies the returned buckets. It seems that this would be possible by doing a bucket copy (which should always succeed since we are dealing with already-read data). - Let's consider how core_input_filter would handle this: Have it treat it as a *readbytes != 0 case with the following exception: it will copy *all* buckets in b back to ctx-b before returning. - mod_ssl: It would convert it from an AP_MODE_PEEK read to a *readbytes (AP_IOBUFSIZE) and read the data normally. In ssl_io_filter_Input, it will do as core_input_filter does and leave the data in ctx-b to be read again (and return it too). Note that core_input_filter will *not* see this read as a PEEK, but as a normal ap_get_brigade call (*readbytes != 0). - HTTP_IN: Gets out of its way and lets it be passed down without any verification on its part. (Should it be concerned about reading past the end-of-request??) - xlate_in_filter: Can save the buffer in its context for later reading. 11. Was any data read? - If not, translate the buffer from ascii and return. 12. Is the first character a tab or space? - If not, translate the buffer from ascii and return. 13. Go to step 1.
Re: Logs and logs and logs [oh my!]
On Sun, 30 Dec 2001, William A. Rowe, Jr. wrote:
Isn't it time to drop TransferLog and CookieLog?
We can accomplish the same by allowing that LogFormat provides the default
for the CustomLog directive, in the absense of an optional [format] arg.
And if we offer a built-in (or default-config'ed) 'cookie' format,
of %{Cookie}n \%r\ %t, then it's a two bit change to turn a CookieLog
into a CustomLog file cookie command.
One of the major 'bugs' in Apache is it's configurability ... I don't think
we want to perpetuate the many-directives-to-accomplish-the-same-thing
methods. Folks are making minor changes already to their configs, this is
as good a time as any to jettison some of the extra cruft :)
I'm not convinced it is any simpler to have one directive with a
whole bunch of options as opposed to a simple version of a directive,
which does what most people want, and then another one that lets them
do the complex stuff.
The complexity of configuration isn't defined by the number of
directives. I think that requiring people to specify more options
to perform the simple case of configuration contributes to people being
intimidated by Apache's flexibility even when they don't want to use that
flexibility. The ability to use a custom logging format shouldn't
require that every user understand that ability just to use the default
format that most people use.
Regardless, the name CustomLog just seems completely wrong for
the default format, and if directives are changing around then adding
another legacy (well, it is called customlog because there used to be
another one and...) doesn't make sense.
I would recommend either having what we have now, with a simple
directive (eg. TransferLog) and a more complex one (eg. CustomLog), or
having a simple form of the directive (eg. TransferLog filename)
and a more complex form (eg. Transferlog filename format). People
shouldn't have to specify a format for the simple case.
While it is simpler to do away with special cases from the code's
perspective, that isn't the user's perspective.
I agree with Marc.
Bill (who has no time for a more lengthly response :-)
Re: Input Filtering and Pushback.
On Monday 31 December 2001 02:40 am, Justin Erenkrantz wrote: I've attempted to think some more about how we should handle pushback in the input filtering (say that a caller does a speculative read - i.e. MIME-continuation in ap_[r]getline). First of all, this isn't pushback at all. Pushback means that you are taking data that you have read, and are handing it back to a lower filter. That doesn't exist in Apache's filtering model, and it isn't necessary. I think the best strategy would be to introduce a new input filter mode (call it PEEK and rename the current PEEK mode to EATCRLF) that I would recommend not re-using the PEEK mode, because that will cause confusion to people who have written input filters already. will indicate to input filters that this data may not be consumed and that it should hold on to the data if possible so that it can be read again (on a non-PEEK call). Using this strategy wisely should remove all instances of a req_cfg-bb (such as what ap_[r]getline uses). Data may only be stored inside the filter chain (no external brigades with unread input are allowed!). What follows is my high-level algorithm on how ap_[r]getline would work with this strategy. Please provide any feedback or thoughts you may have on this. Thanks. -- justin ap_getline with buckets and brigades: 1. call ap_get_brigade with 0 to indicate want LF-line. - ap_get_brigade may return with a line that does not have a LF-ending (i.e. len == HUGE_STRING_LEN). 2. Check to see if we got an error value back (!= APR_SUCCESS) 3. Call apr_brigade_length to determine length of brigade. 4. Do we have enough space in our caller's buffer to handle this? (If we're not doing the allocation for them, that is.) 5. Run through all buckets in the brigade and read/copy into the caller's buffer. (What happened to zero-copy?) We don't do zero-copy on input. The real benefit to zero-copy is on output, because the data tends to be larger. 6. Does last bucket's data (APR_BRIGADE_LAST) have LF char? - If no LF, go to step 1. 7. We now have a LF-line (otherwise, we'd error out). 8. Attempt to compress the EOL by removing any CR or whitespace. - This would maintain backwards compatibility, but may no longer be worthwhile. 9. Are we asked to handling folding? - If not, then translate the buffer from ascii and return. 10. Call ap_get_brigade with PEEK. - Should this PEEK call block? Is that an option that the caller to ap_get_brigade specifies? I think ap_getline would indeed want it to be a blocking PEEK. If we are going to conitnue to extend input filtering, we should probably split the mode from the blocking. - This peek is not the same as current AP_MODE_PEEK (which just eats CRLFs). It would require some modifications to the current input filters to ensure that each filter does *NOT* delete the returned data from its input stack, but instead copies the returned buckets. It seems that this would be possible by doing a bucket copy (which should always succeed since we are dealing with already-read data). - Let's consider how core_input_filter would handle this: Have it treat it as a *readbytes != 0 case with the following exception: it will copy *all* buckets in b back to ctx-b before returning. - mod_ssl: It would convert it from an AP_MODE_PEEK read to a *readbytes (AP_IOBUFSIZE) and read the data normally. In ssl_io_filter_Input, it will do as core_input_filter does and leave the data in ctx-b to be read again (and return it too). Note that core_input_filter will *not* see this read as a PEEK, but as a normal ap_get_brigade call (*readbytes != 0). - HTTP_IN: Gets out of its way and lets it be passed down without any verification on its part. (Should it be concerned about reading past the end-of-request??) - xlate_in_filter: Can save the buffer in its context for later reading. 11. Was any data read? - If not, translate the buffer from ascii and return. 12. Is the first character a tab or space? - If not, translate the buffer from ascii and return. 13. Go to step 1. Since the PEEK function is returning data now, you will need to compare the data read through the PEEK mode to the next chunk of data read to be sure that they are identical. Other than that, it looks good. Ryan __ Ryan Bloom [EMAIL PROTECTED] Covalent Technologies [EMAIL PROTECTED] --
Re: Input Filtering and Pushback.
Ryan Bloom wrote: We don't do zero-copy on input. We've been getting closer to zero-copy on input with recent optimizations like ap_rgetline(). We still do at least two copies of each request header. I don't know whether it's worthwhile to try to further reduce the copying, but I'll find out the next time I have a chance to look at profiler data. (Now that a lot of bigger performance problems in 2.0 have been solved, little things like this sometimes account for a large enough percentage of CPU time to be worth fixing.) --Brian
Re: [PATCH] mod_proxy truncates status line
On Sun, Dec 30, 2001 at 02:58:16PM +0200, Graham Leggett wrote: Adam Sussman wrote: Mod_proxy truncates the status line returned by the proxied server. One character gets snipped off of the end of the status line. Are you 100% sure the buffer is big enough to do this? If the buffer is of size len the zero will be written past the end of the buffer. In the current code, len is strlen(buffer) ... Which means that we can just remove the assignment entirely. buffer is already NULL terminated else strlen would be blowing up. In fact, ap_proxy_string_read() NULL terminates the returned buffer. Bill
Re: [PATCH] mod_proxy infinite cpu eating loop
Adam,
I am leaving ap_proxy_string_read() in place for now. I think the 'right' thing to do
is
to use ap_getline() but I havent the time to figure out how to replace
ap_proxy_string_read() in the proxy_ftp code (no request_rec and ap_getline requires a
request_rec).
I am checking in your fixes with some small tweaks here and there as needed.
Bill
hmm, so I tried out this patch and found that it does work correctly
for most cases and it does solve the original infinite loop problem.
However, it appears to have introduced a new infinite loop problem
as well as some truncation of proxy data.
Once status and header data have been read (or attempted to be read
in the case of HTTP/0.9), mod_proxy is busy waiting for body content.
This shows up as 100% cpu on my setup. The loop where this is happening
is based on a non-blocking call to ap_get_brigade() in proxy_http.c:856.
Can anyone tell me why this call should not block?
In the case of a HTTP/0.9 response, the line feed on the first line
(where status is tested) is eaten and never shows up in the output.
I suspect that is because of ap_rgetline().
Lastly, proxy_ftp also uses ap_proxy_string_read and will need to be
dealt with if we trash that function.
-adam
On Sat, Dec 29, 2001 at 08:02:32AM -0500, Bill Stoddard wrote:
I spent a bit of time looking at this one and I am pretty sure this is not the
right
patch. The problem is that ap_proxy_string_read() is completely broken. Among other
things, it completely chokes if the 'string' spans multiple brigades.
ap_proxy_string_read should be trashed and something like this patch should be used
instead (not tested):
Index: proxy_http.c
===
RCS file: /home/cvs/httpd-2.0/modules/proxy/proxy_http.c,v
retrieving revision 1.114
diff -u -r1.114 proxy_http.c
--- proxy_http.c 19 Dec 2001 16:32:01 - 1.114
+++ proxy_http.c 29 Dec 2001 12:57:09 -
@@ -657,6 +657,22 @@
while (received_continue) {
apr_brigade_cleanup(bb);
+while ((len = ap_getline(buffer, sizeof(buffer), rp, 0)) = 0) {
+if (len 0) {
+/* return status... what? timeout? connection dropped?
+ * for now, just use what was returned in the original broken code
+ * set rp-aborted?
+ */
+apr_socket_close(p_conn-sock);
+backend-connection = NULL;
+ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
+ proxy: error reading status line from remote
+ server %s, p_conn-name);
+return ap_proxyerror(r, HTTP_BAD_GATEWAY,
+ Error reading from remote server);
+}
+}
+#if 0
if (APR_SUCCESS != (rv = ap_proxy_string_read(origin, bb, buffer,
sizeof(buffer),
eos))) {
apr_socket_close(p_conn-sock);
backend-connection = NULL;
@@ -667,7 +683,7 @@
Error reading from remote server);
}
len = strlen(buffer);
-
+#endif
/* Is it an HTTP/1 response?
* This is buggy if we ever see an HTTP/1.10
*/
- Original Message -
From: Adam Sussman [EMAIL PROTECTED]
To: [EMAIL PROTECTED]
Sent: Friday, December 28, 2001 8:24 PM
Subject: [PATCH] mod_proxy infinite cpu eating loop
ap_proxy_string_read currently goes into an infinite loop when the proxied server
closes the connection without sending any data. This patch fixes the problem
but I am not sure that this is the right way to do it.
-adam
Index: modules/proxy/proxy_util.c
===
RCS file: /home/cvspublic/httpd-2.0/modules/proxy/proxy_util.c,v
retrieving revision 1.73
diff -u -r1.73 proxy_util.c
--- modules/proxy/proxy_util.c 28 Nov 2001 21:07:32 - 1.73
+++ modules/proxy/proxy_util.c 29 Dec 2001 00:14:18 -
@@ -1039,6 +1039,7 @@
APR_BUCKET_REMOVE(e);
apr_bucket_destroy(e);
}
+if (APR_BRIGADE_EMPTY(bb)) break;
}
return APR_SUCCESS;
--
I believe in Kadath in the cold waste, and Ultima Thule. But you
cannot prove to me that Harvard Law School actually exists.
- Theodora Goss
I'm not like that, I have a cat, I don't need you.. My cat, and
about 18 lines of bourne shell code replace you in life.
- anonymous
Adam Sussman
Vidya Media Ventures
[EMAIL PROTECTED]
Re: httpd.conf Qs
On Sun, Dec 30, 2001 at 01:03:28PM -0500, Dale Ghent wrote: 2) Quite a while ago, Martin committed PNG format analogs of the GIFs in the icons directory. Was the original plan to move from the GIF format to PNG for these? Should I include a patch for httpd.conf to do this, as well? Quite a bit of space would be reduced in the 2.0 tar ball if GIF were elimintated. No, GIF will still be the default for quite some time. Roy
Directory completion?
If I add a directory to my httpd.conf and restart apache, or I add a directory to htdocs, then point my browser as such: http://127.0.0.1/somedir It hangs until the browser times out. If I do the following though, without restarting apache, etc: http://127.0.0.1/somedir/ it works. Why is that, and what am I missing. It seems so simple, yet I don't understand what option I'm missing. Thanks for the time, and happy new-year! -- Austin Gonyou Systems Architect, CCNA Coremetrics, Inc. Phone: 512-698-7250 email: [EMAIL PROTECTED] It is the part of a good shepherd to shear his flock, not to skin it. Latin Proverb signature.asc Description: This is a digitally signed message part
Re: cvs commit: apr/misc/win32 rand.c
From: Roy T. Fielding [EMAIL PROTECTED]
Sent: Monday, December 31, 2001 4:16 PM
On Sun, Dec 30, 2001 at 07:50:47PM -, [EMAIL PROTECTED] wrote:
--- rand.c 29 Dec 2001 03:33:49 - 1.11
+++ rand.c 30 Dec 2001 19:50:46 - 1.12
@@ -62,8 +62,10 @@
HCRYPTPROV hProv;
apr_status_t res = APR_SUCCESS;
+/* 0x40 bit = CRYPT_SILENT, only introduced in more recent PSDKs
+ */
if (!CryptAcquireContext(hProv, NULL, NULL, PROV_RSA_FULL,
- CRYPT_VERIFYCONTEXT | CRYPT_SILENT)) {
+ CRYPT_VERIFYCONTEXT | 0x40)) {
Eh? Why didn't you do
#ifndef CRYPT_SILENT
#define CRYPT_SILENT 0x40
#endif
Suppose that would work just as well...
I'm actually considering
#ifndef CRYPT_SILENT
#define CRYPT_SILENT 0x40
#elif CRYPT_SILENT != 0x40
#error The CRYPT_SILENT define has changed... please correct and rebuild
#endif
and moving the whole lot of them [several others used in file_io] into a
winfixsdk.h hiding over in apr/misc/win32/. Comments?
Bill
Re: Directory completion?
On Mon, Dec 31, 2001 at 05:36:22PM -0600, Austin Gonyou wrote: If I add a directory to my httpd.conf and restart apache, or I add a directory to htdocs, then point my browser as such: http://127.0.0.1/somedir It hangs until the browser times out. If I do the following though, without restarting apache, etc: My guess is your ServerName is set to something that doesn't exist from the perspective of your browser. When you request http://127.0.0.1/somedir it generates a 301 Moved Permanently to whatever was in your ServerName (plus the path and some other stuff), at which point your browser tries to resolve that new host. If it cannot it will hang until timeout. http://127.0.0.1/somedir/ This one doesn't generate a 301 since it is a properly formed URL (it references a real resource). it works. Why is that, and what am I missing. It seems so simple, yet I don't understand what option I'm missing. -aaron
Re: Directory completion?
From: Aaron Bannert [EMAIL PROTECTED] Sent: Monday, December 31, 2001 5:53 PM On Mon, Dec 31, 2001 at 05:36:22PM -0600, Austin Gonyou wrote: If I add a directory to my httpd.conf and restart apache, or I add a directory to htdocs, then point my browser as such: http://127.0.0.1/somedir It hangs until the browser times out. If I do the following though, without restarting apache, etc: My guess is your ServerName is set to something that doesn't exist from the perspective of your browser. When you request http://127.0.0.1/somedir it generates a 301 Moved Permanently to whatever was in your ServerName (plus the path and some other stuff), at which point your browser tries to resolve that new host. If it cannot it will hang until timeout. Yes... definately the problem [it bit me just today.] What would the group think about validating that the ServerName/ServerAlias directives resolve to a valid Listen'er of this server? Bill
Re: Directory completion?
On Mon, Dec 31, 2001 at 06:09:56PM -0600, William A. Rowe, Jr. wrote: Yes... definately the problem [it bit me just today.] What would the group think about validating that the ServerName/ServerAlias directives resolve to a valid Listen'er of this server? What about proxies? It is valid to have it not point directly to itself. -- justin
Re: Directory completion?
In my case it has to point to itself for testing purposes. I don't use proxies. On Mon, 2001-12-31 at 18:13, Justin Erenkrantz wrote: On Mon, Dec 31, 2001 at 06:09:56PM -0600, William A. Rowe, Jr. wrote: Yes... definately the problem [it bit me just today.] What would the group think about validating that the ServerName/ServerAlias directives resolve to a valid Listen'er of this server? What about proxies? It is valid to have it not point directly to itself. -- justin -- Austin Gonyou Systems Architect, CCNA Coremetrics, Inc. Phone: 512-698-7250 email: [EMAIL PROTECTED] It is the part of a good shepherd to shear his flock, not to skin it. Latin Proverb signature.asc Description: This is a digitally signed message part
filter registration question
Is there any scenario in which a new filter can be registered at any time after server startup? There are a lot of places in the httpd where we add filters with hardcoded names, like ap_add_output_filter(SUBREQ_CORE, NULL, rnew, rnew-connection); I'd like to look up and cache the filter object for each of these names in a post-config function, to eliminate the name-to-filter mapping during request processing. But that won't work if it's possible for the filter registrations to subsequently change. --Brian
Re: Directory completion?
Chip, You've just described an existing directive; With UseCanonicalName off Apache will form self-referential URLs using the hostname and port supplied by the client if any are supplied (otherwise it will use the canonical name). These values are the same that are used to implement name based virtual hosts, and are available with the same clients. The CGI variables SERVER_NAME and SERVER_PORT will be constructed from the client supplied values as well. - Original Message - From: Chip Turner [EMAIL PROTECTED] To: [EMAIL PROTECTED] Sent: Monday, December 31, 2001 9:27 PM Subject: Re: Directory completion? William A. Rowe, Jr. [EMAIL PROTECTED] writes: On Mon, Dec 31, 2001 at 05:36:22PM -0600, Austin Gonyou wrote: If I add a directory to my httpd.conf and restart apache, or I add a directory to htdocs, then point my browser as such: http://127.0.0.1/somedir It hangs until the browser times out. If I do the following though, without restarting apache, etc: My guess is your ServerName is set to something that doesn't exist from the perspective of your browser. When you request http://127.0.0.1/somedir it generates a 301 Moved Permanently to whatever was in your ServerName (plus the path and some other stuff), at which point your browser tries to resolve that new host. If it cannot it will hang until timeout. Yes... definately the problem [it bit me just today.] What would the group think about validating that the ServerName/ServerAlias directives resolve to a valid Listen'er of this server? Why not base the redirect on the incoming Host: header? That should work in most cases (certainly for the browser, also should work for proxies in front). This way users don't mysteriously hop servers and such. In cases where there is no Host, perhaps fallback to the vhost ServerName. Chip -- Chip Turner [EMAIL PROTECTED] Red Hat Network
RE: Directory completion?
From: William A. Rowe, Jr. [mailto:[EMAIL PROTECTED]] What would the group think about validating that the ServerName/ServerAlias directives resolve to a valid Listen'er of this server? I'd lean towards no. I have a fealing there are semi-valid configs where this wouldn't be true. What about defaulting to UseCanonicalName off (Just an idea...) Joshua.
Re: Directory completion?
From: Joshua Slive [EMAIL PROTECTED] Sent: Monday, December 31, 2001 11:52 PM From: William A. Rowe, Jr. [mailto:[EMAIL PROTECTED]] What would the group think about validating that the ServerName/ServerAlias directives resolve to a valid Listen'er of this server? I'd lean towards no. I have a fealing there are semi-valid configs where this wouldn't be true. What about defaulting to UseCanonicalName off (Just an idea...) Not a bad one... with a big warning; # # Setting UseCanonicalName on with any misconfigured ServerName directives # in the config will cause all external redirects to fail. Check all ServerName # directives in your main server and VirtualHost sections before toggling on. # UseCanonicalName off Oh, and first post [CST]. Happy New Year and a prosperous 2002 to all! Bill
Documenting the Internals
Hi, A new employee of us researches the internals of Apache2 (issues like MPM, POD, some APR stuff, specific MPMs like prefork and NT, etc.). He decided to document everything he learns. Before he spends time, I'd like to know: 1. Are there already documents about these issues? 2. If there is no documentation yet, and he writes his own, is there any need from the public for such documentation? (note: our English is BAD...). -- Eli Marmor [EMAIL PROTECTED] CTO, Founder Netmask (El-Mar) Internet Technologies Ltd. __ Tel.: +972-9-766-1020 8 Yad-Harutzim St. Fax.: +972-9-766-1314 P.O.B. 7004 Mobile: +972-50-23-7338 Kfar-Saba 44641, Israel
