Re: [PATCH] mod_dav streamy error handling
gstein (and others), here is another micro-patch for the original mod_dav.c 1.94 change we made a couple of weeks ago. David Waite noticed a segfault and made this patch. Please apply ASAP! The old code never hit this problem, because it would always send NULL as the last argument to dav_send_multistatus(), *unless* a 404 href response came back from the provider, in which case it sent doc-namespaces. But in that situation, that means doc *had* to be non-NULL; a 404 href can only happen when PROPFIND has a body asking for specific things. --- mod_dav.c 3 Jun 2003 22:09:24 - 1.94 +++ mod_dav.c 19 Jun 2003 20:07:41 - @@ -2051,3 +2051,3 @@ badprops. */ -dav_begin_multistatus(ctx.bb, r, HTTP_MULTI_STATUS, doc-namespaces); +dav_begin_multistatus(ctx.bb, r, HTTP_MULTI_STATUS, doc ? doc-namespaces : NULL);
Re: [PATCH] mod_dav streamy error handling
Applied. Thanks! On Thu, Jun 19, 2003 at 03:17:17PM -0500, Ben Collins-Sussman wrote: gstein (and others), here is another micro-patch for the original mod_dav.c 1.94 change we made a couple of weeks ago. David Waite noticed a segfault and made this patch. Please apply ASAP! The old code never hit this problem, because it would always send NULL as the last argument to dav_send_multistatus(), *unless* a 404 href response came back from the provider, in which case it sent doc-namespaces. But in that situation, that means doc *had* to be non-NULL; a 404 href can only happen when PROPFIND has a body asking for specific things. --- mod_dav.c 3 Jun 2003 22:09:24 - 1.94 +++ mod_dav.c 19 Jun 2003 20:07:41 - @@ -2051,3 +2051,3 @@ badprops. */ -dav_begin_multistatus(ctx.bb, r, HTTP_MULTI_STATUS, doc-namespaces); +dav_begin_multistatus(ctx.bb, r, HTTP_MULTI_STATUS, doc ? doc-namespaces : NULL); -- Greg Stein, http://www.lyra.org/
Re: [PATCH] mod_dav streamy error handling
Applied. Thanks! On Wed, Jun 18, 2003 at 02:52:48PM -0500, Ben Collins-Sussman wrote: [Reposting patch, with gstein's added check for r-sent_bodyct.] Have mod_dav deal with errors that happen during a streamy provider response. * mod_dav.c (dav_method_propfind, dav_method_report): if the dav provider throws an error in the middle of streaming a response, have mod_dav log an error and abort the connection. ... -- Greg Stein, http://www.lyra.org/
Re: [PATCH] mod_dav streamy error handling
OK, so now all the mod_dav 'streamy propfind' patches are in. My two original concerns are addressed: no more segfault, no more unlogged errors. I think jerenkrantz should be happy. gstein and striker as well. That said, it'd be swell if we can get revisions 1.94, 1.95, 1.96 of mod_dav.c backported to httpd-2.0. I hear a rumor that sander is itchin' to release 2.0.47 sometime soon, and Subversion really needs this mod_dav feature. Backport anyone?
RE: [PATCH] mod_dav streamy error handling
From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] Sent: Thursday, June 19, 2003 11:37 PM OK, so now all the mod_dav 'streamy propfind' patches are in. My two original concerns are addressed: no more segfault, no more unlogged errors. I think jerenkrantz should be happy. gstein and striker as well. I am happy. Very. That said, it'd be swell if we can get revisions 1.94, 1.95, 1.96 of mod_dav.c backported to httpd-2.0. I hear a rumor that sander is itchin' to release 2.0.47 sometime soon, Well, itchin' is a bit of a stretch. I would want to see if we can get the periods between releases a bit shorter than last time. and Subversion really needs this mod_dav feature. /me grins That alone is not a good enough reason :) Backport anyone? Yes. I'll update STATUS. Sander
Re: [PATCH] mod_dav streamy error handling
--On Tuesday, June 17, 2003 7:49 PM -0700 Greg Stein [EMAIL PROTECTED] wrote: Hmm. It seems possible that deliver_report could fail before generating any response. Thus, we want to test what the situation is, and deliver the error or perform the new ah, crap. punt option. The logic would be like this: if (!r-sent_bodyct) /* no data sent yet. handle the error normally. */ return dav_handle_err(...) I wonder if r-sent_bodyct could be 0 and there are data in the output buffers that hasn't yet made it down to ap_http_header_filter... -- justin
Re: [PATCH] mod_dav streamy error handling
On Tue, Jun 17, 2003 at 11:33:48PM -0700, Justin Erenkrantz wrote: --On Tuesday, June 17, 2003 7:49 PM -0700 Greg Stein [EMAIL PROTECTED] wrote: Hmm. It seems possible that deliver_report could fail before generating any response. Thus, we want to test what the situation is, and deliver the error or perform the new ah, crap. punt option. The logic would be like this: if (!r-sent_bodyct) /* no data sent yet. handle the error normally. */ return dav_handle_err(...) I wonder if r-sent_bodyct could be 0 and there are data in the output buffers that hasn't yet made it down to ap_http_header_filter... -- justin Definitely possible. But we have no better measure to detect that some output has been generated. Once *some* output has been started (and, say, it has been buffered by some filter), then the only likely error that would come back from deliver_report is some kind of 5xx error. i.e. bad juju. In a properly functioning server, that should never happen, so the exposure to this kind of failure mode is very limited. Presumably, non-fatal error conditions will be tested and checked before the report delivery begins. Those will work just fine. As Ben points out, HTTP status codes are at odds with a streamy output style. That implies total buffering of the output if you want to ensure your status code is always proper. And we know that we don't want to buffer... Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] mod_dav streamy error handling
--On Tuesday, June 17, 2003 11:49 PM -0700 Greg Stein [EMAIL PROTECTED] wrote: Definitely possible. But we have no better measure to detect that some output has been generated. Once *some* output has been started (and, say, it has been buffered by some filter), then the only likely error that would come back from deliver_report is some kind of 5xx error. i.e. bad juju. In a properly functioning server, that should never happen, so the exposure to this kind of failure mode is very limited. How bad would it be if we incorrectly think that no body has been sent when one has been pushed into the chain? Is it just going to be a 207 with a corrupted XML response - the error body would be appended to whatever has already been sent? If we guess right (that there has been a body), then it'd be a 207 with the first portion and then a closed connection. So, either case, the client is just hosed. Perhaps it makes sense to always emit the error 'body' to the client, then abort the connection. That way, there is some hint that something is majorly wrong in the server by looking at the content. Not entirely sure. -- justin
Re: [PATCH] mod_dav streamy error handling
[Reposting patch, with gstein's added check for r-sent_bodyct.]
Have mod_dav deal with errors that happen during a streamy provider response.
* mod_dav.c (dav_method_propfind, dav_method_report): if the dav
provider throws an error in the middle of streaming a response, have
mod_dav log an error and abort the connection.
Index: modules/dav/main/mod_dav.c
===
RCS file: /home/cvspublic/httpd-2.0/modules/dav/main/mod_dav.c,v
retrieving revision 1.94
diff -u -r1.94 mod_dav.c
--- modules/dav/main/mod_dav.c 3 Jun 2003 22:09:24 - 1.94
+++ modules/dav/main/mod_dav.c 18 Jun 2003 19:47:44 -
@@ -2059,8 +2059,18 @@
}
if (err != NULL) {
-/* ### add a higher-level description? */
-return dav_handle_err(r, err, NULL);
+/* If an error occurred during the resource walk, there's
+ basically nothing we can do but abort the connection and
+ log an error. This is one of the limitations of HTTP; it
+ needs to know the entire status of the response before
+ generating it, which is just impossible in these streamy
+ response situations. */
+err = dav_push_error(r-pool, err-status, 0,
+ Provider encountered an error while streaming
+ a multistatus PROPFIND response., err);
+dav_log_err(r, err, APLOG_ERR);
+r-connection-aborted = 1;
+return DONE;
}
/* Finish up the multistatus response. */
@@ -4033,9 +4043,22 @@
/* run report hook */
if ((err = (*vsn_hooks-deliver_report)(r, resource, doc,
r-output_filters)) != NULL) {
-/* NOTE: we're assuming that the provider has not generated any
- content yet! */
-return dav_handle_err(r, err, NULL);
+if (! r-sent_bodyct)
+ /* No data has been sent to client yet; throw normal error. */
+ return dav_handle_err(r, err, NULL);
+
+/* If an error occurred during the report delivery, there's
+ basically nothing we can do but abort the connection and
+ log an error. This is one of the limitations of HTTP; it
+ needs to know the entire status of the response before
+ generating it, which is just impossible in these streamy
+ response situations. */
+err = dav_push_error(r-pool, err-status, 0,
+ Provider encountered an error while streaming
+ a REPORT response., err);
+dav_log_err(r, err, APLOG_ERR);
+r-connection-aborted = 1;
+return DONE;
}
return DONE;
RE: [PATCH] mod_dav streamy error handling
From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] Sent: Tuesday, June 17, 2003 8:27 PM Have mod_dav deal with errors that happen during a streamy provider response. (This is a followup to the change that makes dav_method_propfind streamy.) Hopefully, this addresses jerenkrantz's concerns; gstein and I can't come up with a better solution, given the limits of HTTP design. * mod_dav.c (dav_method_propfind, dav_method_report): if the dav provider throws an error in the middle of streaming a response, have mod_dav log an error and abort the connection. This seems to be the only viable solution. +1. Sander
Re: [PATCH] mod_dav streamy error handling
On Tue, Jun 17, 2003 at 01:27:19PM -0500, Ben Collins-Sussman wrote:
...
@@ -4033,9 +4043,19 @@
/* run report hook */
if ((err = (*vsn_hooks-deliver_report)(r, resource, doc,
r-output_filters)) != NULL) {
-/* NOTE: we're assuming that the provider has not generated any
- content yet! */
Hmm. It seems possible that deliver_report could fail before generating any
response. Thus, we want to test what the situation is, and deliver the error
or perform the new ah, crap. punt option.
The logic would be like this:
if (!r-sent_bodyct)
/* no data sent yet. handle the error normally. */
return dav_handle_err(...)
/* damn. data was sent. bail. */
...
This logic isn't needed for the PROPFIND case because we always send the
initial leader for the multistatus response. So... since we've always sent
data, then we need to just bail.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
