Developers of Sympa

[Guillaume Rousse <address@concealed>]

Le 07/12/2012 12:23, IKEDA Soji a écrit :
> David,
>
> On Fri, 07 Dec 2012 11:03:07 +0100
> David Verdin <address@concealed> wrote:
>
> > Hi guys,
> >
> > I see an alarmingly growing number of croak calls in the Sympa code. I
> > think I misunderstand what this function does.
> > As far as I understand it, it simply kills the program after having
> > issued a last log. However, it looks like Sympa doesn't die any time
> > corak is called, so I think I don't fully understand what exactly is the
> > effect of croak.
> >
> > In which cases, in our code, will croak actually kill the process?
> > Because we must not kill our processes on errors. Errors will happen all
> > the time. We must warn users, send mail, but never kill the processes,
> > except if they can't access vital ressources, such as database.
>
> Are you by any chance talking about $SIG{__DIE__} hook I added?
> (see bottom of src/lib/Site.pm)  This hook is called anytime
> process is dying (either by "die" or by "croak"), does some extra
> works, and then really dies.
>
> On current Sympa-6.2, dying processes output traceback to STDERR
> and (if possible) to syslog.  I believe debugging of the sudden
> deaths will be made easier.
I think we need a consistent plan about error management here...

There is a first discussion about error recovery.

There is little point in ensuring our codes survives and continue to run 
after any kind of error. A large class of such errors are due to 
developpement issues, for instance a missing parameter (provided the 
parameter list is not computed dynamically), or some type error after 
heavy refactoring. That's exactly the kind of error a statically typed 
language compiler would catch, but unfortunatly for us we have rely on 
testing (provided enough coverage), or runtime assertions, to do the 
same. I'm OK to consider those errors as unrecoverable (as unexpected, 
and fixables), and the current strategy of handling them gracefully 
through a top-level signal handler seems fine.

I'm not convinced, tough, about the interest of generating a backtrace 
ourselve in the signal handler, wheras confess() function should work as 
well.

Now, there is a second discussion about recoverable errors handling. If 
we start using exceptions for fatal errors, we may as well use them also 
for recoverable ones, instead of the current 'return undef if something 
went wrong' strategy.

For instance, the following current code looks like:

package Upgrade;

use Log;

unless (tools::set_file_rights(
    file  => $file,
    user  => Sympa::Constants::USER,
    group => Sympa::Constants::GROUP,
    mode  => 0644,
)) {
    Log::do_log('err','Unable to set rights on template file %s', $file);
    next;
}

package tools;

use Log;

sub set_file_rights {
    unless (chmod($param{'mode'}, $param{'file'})) {
        &Log::do_log('err', "Can't change rights of file %s: %s", 
 $params{'file'}, $!);
        return undef;
    }
}

Error handling and reporting is done twice.

Now, consider this code:

package Upgrade;

use Log;

eval {
    set_file_rights($mode, $file);
}
if ($EVAL_ERROR) {
    Log::do_log('err', "Error while handling configuration file %s: 
%s", $file, $EVAL_ERROR);
}

package tools;

sub set_file_rights {

    chmod($param{'mode'}, $param{'file'))
        or die "Can't change rights of file %s: %s", $file, $!);
}

This would have multiple advantages:
1) no more "everything requires the Log module because every piece of 
code has to report its error itself", which plays a large part in 
current cross-dependencies deadlock

2) easier unit-testing, as the caller code knows what is the error, 
instead of just being noticed an error occured

3) better synthetic error messages.

The current code currently results in two error messages, each reporting 
a different part of the problem:

tools:: set_file_rights(): Can't change rights of file foobar: 
permission denied
Upgrade::some_function(): Unable to set rights on template file foobar

Handling error only once would result in a single message, with every 
piece of information at once:
Upgrade::some_function(): Unable to set rights on template file foobar: 
Can't change rights of file foobar: permission denied

Note tough than exceptions are not the only way to achieve this strategy 
of moving back error handling upstream in the contol flow, just probably 
the most convenient.

--
Guillaume Rousse
INRIA, Direction des systèmes d'information
Domaine de Voluceau
Rocquencourt - BP 105
78153 Le Chesnay
Tel: 01 39 63 58 31

Attachment: smime.p7s
Description: Signature cryptographique S/MIME