schame...@spinor.com wrote on Sat, Dec 24, 2011 at 09:46:41 +0100:
> On 2011-12-23 23:52, Branko Čibej wrote:
> >On 23.12.2011 18:49, Stefan Küng wrote:
> >>On 23.12.2011 15:18, Stefan Sperling wrote:
> >>>On Fri, Dec 23, 2011 at 04:10:07PM +0200, Daniel Shahaf wrote:
> >>>>On Fri, Dec 23, 2011, at 14:50, Stefan Sperling wrote:
> >>>>>On Fri, Dec 23, 2011 at 02:26:40PM +0200, Daniel Shahaf wrote:
> >>>>>>On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
> >>>>>>>On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
> >>>>>>>>s...@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> >>>>>>>>>+ An informative error message helps people more than an
> >>>>>>>>>assert (which
> >>>>>>>>>+ in case of TSVN can mean crashing the Windows Explorer).
> >>>>>>>>
> >>>>>>>>I thought the svn_error_malfunction_handler_t stuff addressed
> >>>>>>>>that issue?
> >>>>>>>
> >>>>>>>No, because TSVN didn't end up using it as intended.
> >>>>>>>See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
> >>>>>>
> >>>>>>I don't see how that's relevant? That link discusses the error
> >>>>>>dialogs.
> >>>>>>What I'm asking is whether TSVN has implemented an
> >>>>>>svn_error_malfunction_handler_t that replaces the default handler,
> >>>>>>svn_error_abort_on_malfunction().
> >>>>>
> >>>>>Yes it does (called svn_error_handle_malfunction()) but it calls
> >>>>>abort()
> >>>>>which means the explorer crashes:
> >>>>>http://tortoisesvn.googlecode.com/svn/trunk/src/SVN/SVN.cpp
> >>>>
> >>>>And they can't remove the abort() call because...?
> >>>
> >>>As Stefan explained, he cannot rely on the internal state of the
> >>>application if he gets an assertion thrown out of the svn libraries.
> >>>
> >>>And he has a point. He assumes that asserts are fatal and
> >>>non-recoverable
> >>>errors. What if Subversion asserts because it just trashed the entire
> >>>stack and heap space of the windows explorer and happens to operate on
> >>>garbaga data that triggers an assertion? It doesn't make any sense to
> >>>try
> >>>to continue.
> >>>
> >>>Now, we know that most of our assertions are due to bugs in our code
> >>>where, for instance, invalid data entered wc.db. But an application
> >>>using the svn libraries cannot safely rely on this knowledge.
> >>>Changing all these bogus asserts that trigger just because of invalid
> >>>working copy meta-data into proper errors is the right way to fix this.
> >>
> >>I think I have to explain something here:
> >>First, the shell extension of TSVN does not even call
> >>svn_error_set_malfunction_handler() to set its own handler. Since that
> >>function clearly states that "This function must be called in a
> >>single-threaded context", I can not initialize that in the shell
> >>extension since the extension can get loaded in multi threaded
> >>situations.
> >>And the default handler asserts and calls abort(). So the whole shell
> >>goes down in such situations.
> >>
> >>Then: the reason I call abort() in my own handler for TortoiseProc
> >>(the UI part of TSVN) is that the SVN also clearly state that "If
> >>can_return is false the method should never return. (The caller will
> >>call abort())".
> >>So what else than call abort() can I do? I can not return from that
> >>function if can_return is false. No way to exit but to call abort().
> >>So how is that "not implemented as intended"???
> >>At least now in the UI part I can show a nasty dialog box telling the
> >>users where to report the problem to. That way it's not me that gets
> >>bothered with these since I can't do anything about them anyway.
> >>
> >>I have _repeatedly_ mentioned on this list that this kind of "error
> >>handling" (quotes intended) is bad. Sure, it won't affect the CL
> >>client that much, but for a library that's just not acceptable.
> >>But others have similar error handling implemented:
> >>http://thedailywtf.com/Articles/Serious-Failure.aspx
> >
> >Ranting is all very well, but I've yet to hear a suggestion from you
> >about how the libraries should handle unrecoverable errors. Like, for
> >example, the case where wc.db contains inconsistent and/or invalid data.
>
> Simple answer:
> Return an normal error, giving the caller (e.g. TSVN shell extension)
> the chance for example to continue its work on other working copies.
> But definitely never just abort or assert (crash the Windows explorer).
>
How is calling svn_error_malfunction_handler_t(can_return=TRUE) not such
a mechanism?
When can_return=FALSE then the library itself abort()s if the callback
does return... but when can_return=TRUE, the library is supposed to
propagate the error all the way up[1]
[1] though, in practice, we may have places that do
if (err && err->apr_err == FOO)
/* .. */
else
svn_error_clear(err);
which would mask assertions. Perhaps we should make it a fatal error to
call svn_error_clear() on an error if
SVN_ERROR_IN_CATEGORY(SVN_ERR_MALFUNC_CATEGORY_START) ?
> So there must be never "unrecoverable errors".
> There may be some unrecoverable data (e.g. a broken db/WC),
> but the caller must be able to continue work on other data
> (e.g. another WC).
>
+1
> Cheers,
> Folkr
