You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ben Collins-Sussman <su...@collab.net> on 2003/06/17 20:27:19 UTC
[PATCH] mod_dav streamy error handling
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.
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 -0000 1.94
+++ modules/dav/main/mod_dav.c 17 Jun 2003 18:23:44 -0000
@@ -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,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! */
- 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
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Wednesday, June 18, 2003 12:47 AM -0700 Greg Stein <gs...@lyra.org>
wrote:
> I think dropping the connection on the client rather than sending a bogus
> body is probably "better". Neither solution is great, but I'll take a drop
> and a truncated response over a bad body.
Sure, I can live with that. +1.
The server error logs should have the error details anyway. -- justin
Re: [PATCH] mod_dav streamy error handling
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Jun 18, 2003 at 12:03:50AM -0700, Justin Erenkrantz wrote:
> --On Tuesday, June 17, 2003 11:49 PM -0700 Greg Stein <gs...@lyra.org>
> 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?
Yes. That is the behavior today. Ben is trying to clean that up. The
proposed patch reduces the chance of these mixed response/error bodies.
> 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.
Yup.
> 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.
I think dropping the connection on the client rather than sending a bogus
body is probably "better". Neither solution is great, but I'll take a drop
and a truncated response over a bad body.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] mod_dav streamy error handling
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Tuesday, June 17, 2003 11:49 PM -0700 Greg Stein <gs...@lyra.org> 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
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Jun 17, 2003 at 11:33:48PM -0700, Justin Erenkrantz wrote:
> --On Tuesday, June 17, 2003 7:49 PM -0700 Greg Stein <gs...@lyra.org>
> 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
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Tuesday, June 17, 2003 7:49 PM -0700 Greg Stein <gs...@lyra.org> 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
Posted by Greg Stein <gs...@lyra.org>.
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/
Re: [PATCH] mod_dav streamy error handling
Posted by Greg Stein <gs...@lyra.org>.
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 -0000 1.94
> +++ mod_dav.c 19 Jun 2003 20:07:41 -0000
> @@ -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
Posted by Ben Collins-Sussman <su...@collab.net>.
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 -0000 1.94
+++ mod_dav.c 19 Jun 2003 20:07:41 -0000
@@ -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
Posted by Sander Striker <st...@apache.org>.
> From: sussman@collab.net [mailto:sussman@collab.net]
> 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
Posted by Ben Collins-Sussman <su...@collab.net>.
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
Posted by Greg Stein <gs...@lyra.org>.
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
Posted by Ben Collins-Sussman <su...@collab.net>.
[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 -0000 1.94
+++ modules/dav/main/mod_dav.c 18 Jun 2003 19:47:44 -0000
@@ -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
Posted by Sander Striker <st...@apache.org>.
> From: sussman@collab.net [mailto:sussman@collab.net]
> 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