[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Better error reporting from mod_dav_svn?

From: Karl Fogel <kfogel_at_red-bean.com>
Date: Thu, 14 Feb 2008 00:36:40 -0500

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

This is an archived mail posted to the Subversion Dev mailing list.