2011/12/23 Stefan Küng <tortoise...@gmail.com>: > 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. >
Please excuse my naive questions, coming from outside That "must be called in a single-threaded context" in the doc - is it actually a usage constraint? Maybe it is just wrong documentation? Looking at the code of that method in error.c, it just replaces a value of a static variable with a new one and returns the old value. Is that unsafe? [1] http://svn.apache.org/viewvc/subversion/branches/1.7.x/subversion/libsvn_subr/error.c?view=markup#l656 Other question is what to do in such a malfunction handler method. Can one use Windows exception handling [2][3] ? or C's longjmp? [2] http://msdn.microsoft.com/en-us/library/swezty51%28v=vs.80%29.aspx [3] http://msdn.microsoft.com/en-us/library/windows/desktop/ms680552%28v=vs.85%29.aspx Does SVN library start its own threads for a single API call? (Returning from the same thread is doable, but how does one return from a different thread started from the main one?) I suppose all of the above is not compatible with APR library, and I would expect leaked resources, but it might be better than a crash. > > 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 > > Best regards, Konstantin Kolinko
