Subversion's mod_dav_svn module currently uses a default malfunction handler, svn_error_abort_on_malfunction(). This means that SVN_ERR_MALFUNCTION() or failed SVN_ERR_ASSERT() statements output the malfunction details to STDERR and abort() the current process. Doing so within an Apache module — as opposed to a command-line program like svn — has two potential problems:
A) Logging A default malfunction handler calls svn_handle_error2(), which prints the malfunction details to STDERR. Apache replaces STDERR with what ErrorLog directive [1] points to, and the information about a malfunction goes right to it. However, a couple of caveats apply. First of all, doing so bypasses existing error log hooks installed via ap_hook_error_log(). I think that there might be existing installations that happen to use custom error log hooks as an addition to the main error log or even as a complete replacement for it (think ErrorLog /dev/null, but with a custom logging module doing all the work). In the latter case, the information about the malfunction would be completely lost. Secondly, the error message contains an unnecessary "svn:" prefix, and this is inconsistent with other Subversion errors that can appear in the Apache error log. B) Taking down other threads Calling abort() means we take down the entire process, with a respect to the currently chosen MPM. We cannot assume that this process does not contain other, non-mod_dav_svn threads and we might want to be conservative about abort()'s here, as just killing everything could do more harm than good. Here are several possible options: 1) Install a mod_dav_svn malfunction handler that calls ap_log_assert(); ap_log_assert() also terminates the current process, so this means we would keep the existing abortive behavior. Pros: Solves A) Cons: Doesn't really do anything about B) 2) Install a non-abortive mod_dav_svn malfunction handler, which would log the malfunction details using ap_log_error() and return SVN_ERR_ASSERTION_FAIL when CAN_RETURN = TRUE. This option is similiar to the current behavior of a non-default malfunction handler, svn_error_raise_on_malfunction(). (See svn_error_malfunction_handler_t for details about CAN_RETURN argument) Pros: Doing so would address both A) and a huge subset of B). Only a subset, because there still are situations with CAN_RETURN = FALSE when we really cannot do something graceful in a malfunction handler rather than abort(). Cons: Our codebase currently includes ~ 300 assertions, which might eventually get triggered by mod_dav_svn. We might encounter situations with the callers ignoring an SVN_ERR_ASSERTION_FAIL. Say, they could svn_error_clear() it. Or turn it into dav_error *, and then mod_dav would ignore this error for some reasons and continue to call us as if nothing happened. This could result in us running in an undefined state — i.e., where abort() would have probably been a better choice. 3) Add a SVNAbortOnMalfunction directive (default is On). Install different malfunction handlers based on the value of this flag. Describe what's the purpose of this directive and announce an intention to change the default to Off in the next minor release. Let it soak, and change the default. Pros: Same as 2), but an administrator can make a better choice based on his/her Apache configuration. Cons: Requires time. Does not address B) with the default value. Thoughts? [1] http://httpd.apache.org/docs/2.4/mod/core.html#errorlog Regards, Evgeny Kotkov
