Re: [PATCH] Resolve testsuite interruption in SVN 1.8 (v2)

[Stefan <lu...@posteo.de>]

On 8/8/2016 14:53, Stefan Hett wrote:
> On 8/8/2016 12:22 PM, Ivan Zhakov wrote:
>
>> On 7 August 2016 at 23:14, Stefan <lu...@posteo.de> wrote:
>>
>> Btw what are the problems with approach to disable watson reports on
>> abort(), except backporting? I mean we already override Windows Error
>> Reporting by installing our own SEH exception handler, so it looks
>> natural to disable abort() reporting also.
>>
> I agree with you on this one. I wasn't aware that SVN does already
> alter the default exception handling on Windows (and therefore
> effectively disable the Watson crash dump reports in case of an
> unhanled exception). So disabling the Watson crash dumps on abort
> calls does indeed sound like increasing the system/design consistency
> in SVN.
> You got my non-binding +1 on that proposal (which obviously would be a
> replacement for my proposed patch).
>
I've been thinking more about your arguments and since I agree with you,
I'd like to replace my previously proposed patch with this updated
version based on your idea.
Due to the behavior change it imposes, I'm not going to propose this for
backporting to 1.8 or 1.9, though.

Note that I kept the design to only disable Watson crash reports, unless
SVN_CMDLINE_USE_DIALOG_FOR_ABORT is set. The reasoning is as given in my
previous reply as in as far as I understand the design of that
environment-variable-setting, it should control whether an abort
produces a popup or not. Disabling the Watson crash reports would
prevent that popup (in certain cases) and therefore negate the setting.

[[[
Disable Watson crash reports upon abort()-calls on Windows, to allow
unattended test suite runs if an abort() is triggered.

* subversion/libsvn_subr/cmdline.c
  (svn_cmdline_init): disable Watson crash reports on abort unless
                      SVN_CMDLINE_USE_DIALOG_FOR_ABORT is set

* subversion/tests/svn_test_main.c
  (svn_test_main): the same

Suggested by: ivan
]]]

(Note: This patch was untested, since it's an obvious derivate of the
previous version of the patch.)
Regards,
Stefan

Re: [PATCH] Resolve testsuite interruption in SVN 1.8 (v2)

[Ivan Zhakov <iv...@visualsvn.com>]

On 8 August 2016 at 23:29, Stefan <lu...@posteo.de> wrote:
> On 8/8/2016 14:53, Stefan Hett wrote:
>> On 8/8/2016 12:22 PM, Ivan Zhakov wrote:
>>
>>> On 7 August 2016 at 23:14, Stefan <lu...@posteo.de> wrote:
>>>
>>> Btw what are the problems with approach to disable watson reports on
>>> abort(), except backporting? I mean we already override Windows Error
>>> Reporting by installing our own SEH exception handler, so it looks
>>> natural to disable abort() reporting also.
>>>
>> I agree with you on this one. I wasn't aware that SVN does already
>> alter the default exception handling on Windows (and therefore
>> effectively disable the Watson crash dump reports in case of an
>> unhanled exception). So disabling the Watson crash dumps on abort
>> calls does indeed sound like increasing the system/design consistency
>> in SVN.
>> You got my non-binding +1 on that proposal (which obviously would be a
>> replacement for my proposed patch).
>>
> I've been thinking more about your arguments and since I agree with you,
> I'd like to replace my previously proposed patch with this updated
> version based on your idea.
> Due to the behavior change it imposes, I'm not going to propose this for
> backporting to 1.8 or 1.9, though.
>
> Note that I kept the design to only disable Watson crash reports, unless
> SVN_CMDLINE_USE_DIALOG_FOR_ABORT is set. The reasoning is as given in my
> previous reply as in as far as I understand the design of that
> environment-variable-setting, it should control whether an abort
> produces a popup or not. Disabling the Watson crash reports would
> prevent that popup (in certain cases) and therefore negate the setting.
>
Hi Stefan,

I've tested your patch, but it seems that calling
_set_abort_behavior(0, _CALL_REPORTFAULT) is not enough. The problem
with this approach that abort() uses RaiseFailFastException() that
bypasses all exception handlers [1].

So Subversion doesn't write crash dump and even doesn't print error
message on abort() failure. One solution would be to register signal
handler() and use RaiseException(STATUS_FATAL_APP_EXIT). This
exception would be handled by our own exception handler. Subversion
already uses such approach for handling out-of-memory errors (see
r1724784 [2]). I've tested this approach (see attached patch) and it
seems to be working fine, but I want to test it more before
committing.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd941688(v=vs.85).aspx
[2] https://svn.apache.org/r1724784

-- 
Ivan Zhakov