2011/12/23 Stefan Küng <tortoisesvn_at_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:
>>>>>>>
>>>>>>> stsp_at_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
Received on 2011-12-25 08:15:43 CET