On the mergeinfo-capability branch, r29314 included this change:
Index: subversion/mod_dav_svn/reports/mergeinfo.c
===================================================================
--- subversion/mod_dav_svn/reports/mergeinfo.c (revision 29313)
+++ subversion/mod_dav_svn/reports/mergeinfo.c (revision 29314)
@@ -199,9 +199,25 @@
svn_inheritance_to_word(inherit));
dav_svn__operational_log(resource->info, action);
- /* Flush the contents of the brigade (returning an error only if we
- don't already have one). */
- if ((apr_err = ap_fflush(output, bb)) && !derr)
+ /* If we already have an error, don't even flush the contents of the
+ brigade, because we don't want mod_dav.c:dav_method_report() to
+ think we've sent anything back to the client yet -- as long as it
+ doesn't think that, it'll send the real error to the client,
+ which is what we'd prefer. This situation is described in more
+ detail in httpd-2.2.6/modules/dav/main/mod_dav.c, line 4066, in
+ the comment in dav_method_report() that says:
+
+ 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.
+
+ In other words, flushing the brigade here would cause
+ r->sent_bodyct (see dav_method_report()) to flip from 0 to 1.
+ */
+ if (!derr && (apr_err = ap_fflush(output, bb)))
derr = dav_svn__convert_err(svn_error_create(apr_err, 0, NULL),
HTTP_INTERNAL_SERVER_ERROR,
"Error flushing brigade.",
This change allowed one possible error conditions to get accurately
reported to the client; without this change, the client would just get
an unfinished response body and comprehend it as a generic request
failure, instead of as the specific error I wanted. Even though the
bucket was entirely empty at that point (assuming that point was
reached via the error in question), merely calling flush would set a
flag that... well, you can read the comment above for details.
It turns out that a lot of mod_dav_svn code has a similar conditional
bucket flush (thanks to David Glasser for noticing this), and that in
all of them, the '!derr' test comes after the flush.
I think that we should be testing '!derr' first in all the cases where
this idiom is part of a labeled 'cleanup' block (that is, a possible
destination of a goto), and perhaps also in some cases where it's not.
Whatever amount of body has been sent, flushing the bucket further is
no more likely to finish out a complete, parseable response than it is
to cause a formerly "complete" response to become incomplete; but in
cases where no data has been sent yet, it is *always* better to not
flush the bucket before returning an error. Or so I think.
Really, all those conditionals would have to be evaluated on a
case-by-case basis, and this need not happen for 1.5, so what I'm
really asking is:
* Anyone see a problem with the specific diff above?
* Anyone know a philosophical reason why testing '!derr' first
would just be wrong?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-02-14 06:37:16 CET