Developers of Sympa

[IKEDA Soji <address@concealed>]

Hi Guillaume and all,

On Thu, 03 Oct 2013 16:52:49 +0200
Guillaume Rousse <address@concealed> wrote:

> Le 03/10/2013 10:02, IKEDA Soji a écrit :
> >>>> Exception handling seems not to be resolved. the fatal_err calls were
> >>>> usefull because they specified explicitely that Sympa should not keep 
> >>>> on
> >>>> running under certain circumstances. This is a clear distinction with
> >>>> the return undef we used as exception handling.
> >>>> I am under the impression that all return undef could be replaced by
> >>>> croak calls, providing we intercept exceptions at the right place and 
> >>>> we
> >>>> use the Carp option allowing to produce stak traces.
> >>>> But, we need to distinguish the very few circumstances in which sympa
> >>>> should stop running, so that the daemons will stop gracefully.
> >>>> Inspecting the code, it looks like all fatal_err calls were made in the
> >>>> main daemons, so I think it should be easy.
> >>> My concern on exception:
> >>>
> >>> - It tends to skip clean-up processes inside "try" and other blocks.
> >>>    Not only skipping each logic, it is likly to cause leaks of
> >>>    rosources such as memory.  The function corresponding to "finally"
> >>>    may probably ease such situation, while it can worsen the next
> >>>    problem.
> >> If the "finally" resolves the memory leaks, I violently support this 
> >> option.
> finally does just provides a code place holder, it doesn't magically fix 
> any resource liberation issue.
> 
> >>> - It is a bit hard to read.  The idiom using eval {} and if () is
> >>>    obvious (additionally, by this idiom, programmers must re-throw
> >>>    uncaught exceptions by themselves).  Syntax sugers provided by
> >>>    several modules are not always acceptable by average Perl
> >>>    programmers.
> >> If we set up clear rules about how exceptions are handled in Sympa on
> >> the Sympa web site, it should not be too much trouble.
> >
> > D'accord, I'll try to figure out the rule.
> >
> > IMO to emulate "try-catch" by Perl5, objects will be passed to
> > croak, instead of string.  Here is a template:
> >
> >      # "try" block
> >      eval {
> >
> >          # *1
> >
> >          # Throw exception if error occurred
> >          if (...) {
> >              # *3
> >              croak Sympa::Exception::Foo->new(
> >                  File => __FILE__, Line => __LINE__, Message => 'error 
> > occurred'
> >              );
> >          }
> >
> >          # *2
> >
> >      };
> >
> >      # "catch" block
> >      if (ref $@ eq 'Sympa::Exception::Foo') {
> >
> >          # process exception
> >
> >      } elsif (ref $@ eq 'Sympa::Exception::Bar') {
> >
> >          # process another exception
> >
> >      }
> Testing if an object belongs to a given class should use the ->isa() 
> method, not rely on string comparaison. Otherwise you'll miss subclasses 
> instances.
> if ($@) {
>      if (blessed $@) {
>               if ($@->isa('Sympa::Exception::Foo') {
>          } elsif ($@->isa('Sympa::Exception::Bar') {
>          }
>      } else {
>      }
> }
> 
> 
> >      # uncuaght exceptions
> >      # We have responsibility to re-throw exceptions we won't catch.
> >      elsif (ref $@) {
> >          croak $@;
> >      } elsif ($@) {
> >          croak "$@\n";
> >      }
> >
> > Last two elsif blocks are somewhat eyesore, but they are exactly
> > necessary not to miss exceptions raised by others (maybe in lower
> > level).
> Throwing uncaught exception is a choice, it is not mandatory. The same 
> way you can perfectly ignore result of any function call. And the 
> explicit concatenation of \n character to a string-based exception is 
> cosmetic only, meaning the two block could be merged in to one only:
> } else {
>      croak $@;
> }
> 
> > My first concern: For example, a filehandle is opened at (*1) then
> > closed at (*2).  When exception is thrown, that filehandle can
> > leak.  So we must also close it at (*3).  It is not easy to compile
> > this clean-up process into single place such as "finally" block.
> >
> > # Here, I ignored the alternative to use CPAN modules.
> Using CPAN modules has a cost about your target user base.
> 
> A CPAN module which just provides syntactic sugar (try/catch keywords, 
> instead of eval/if, for instance), is definitively not worth the cost.
> 
> One that allows to propagate objects instead of strings would eventually 
> be more interesting.
> 
> However, there is still room for improvement with current error messages 
> (for instance, having more useful and readable ones), before changing 
> the way they are transmitted.
> 
> > Anyway, my second concern: is the rule above easy enough to keep?
> > (this is a simple question, not rhetorical)
> >
> >
> >>> I'm not an opponent of exception, contrary, I prefer to this
> >>> concept.  But by now I'm undecided.
> >> There is something still unclear to me: How exceptions are escalated. If
> >> I understand correctly, we plan to croak or carp everywhere we used
> >> "return undef" and eval{} these exceptions at the very top level (i.e.,
> >> in each Sympa daemon loops). In the end, will we have only the original
> >> message or all the croak messages we will meet between the original
> >> error and the final eval{} interception?
> >
> > "try-catch" structure described above may appear in several levels,
> > because clean-up processes such as filehandle can be accomplished
> > only in each level.
> >
> > Each "try-catch" catches particular exceptions and processes it ---
> > logs messages, recovers errors etc.  Uncaught exceptions will be
> > escalated (re-thrown) to upper level.  Once an exception has been
> > caught, it no longer will propagate to upper level.
> >
> > Finally at uppermost level, Perl catches exceptions never caught
> > and dies.
> >
> > There is one more comment on implementation.
> >
> > Error messages are contained in exception objects.  Also,
> > information of call stack should be collected when each exception
> > object is instantiated.  These information may be shown as error
> > message and traceback.
> > However, if exception is simple scalar (thrown by external modules or
> > Perl), traceback cannot be shown.  Because information of call stack
> > had been lost.
> I'd rather say: traceback in external code is lost, if you don't 
> intercept them when entering your own code.
> 
> I'm still convinced than we have other priorities than switching error 
> handling to exceptions all over the place right now. And a limited usage 
> of them would probably help figuring out how they work exaclty.

If you use exception in limited usage, you would better propose
coding rule of yours.

Here is the revised rule of mine.  There are some notes after it.

* To throw exception, do:

    die Sympa::Exception::Foo->new(Message => 'error occurred');

* To catch exception, do as below:

    # "try" block
    eval {
        # *1
        # Here is the code probably throws exception
        # *2
    };

    # "catch" block
    if (ref $@ eq 'Sympa::Exception::Foo') {

        # process exception

    } elsif (ref $@ eq 'Sympa::Exception::Bar') {

        # process another exception

    }
    # We have responsibility to re-throw exceptions we won't catch.
    elsif ($@) {
        die $@;
    }

* If processing at (*1) requires clean-up at (*2), make sure that
  such clean-up will be done even when exception was thrown.


Notes (this is not part of rule):

- croak() appends to argument the filename and line number where it
  is called.
  die() doesn't, if the argument ends with "\n".
  If argument is reference, both functions won't modify it.

  So, we may use die() to throw and to re-throw exceptions.

- The function blessed() suggested by Guillaume is useful, but it
  is provided by external package Scalar::Util.

  So tests on exception objects might be done using built-in ref().

- Exception which programmer doesn't wish to catch must be
  re-thrown.  This is the primary rule.

  Since programmers cannot be responsible to the exceptions they
  don't expect, they should not cancel such ones but should pass
  to higher levels.


Again: Is the rule above easy enough to keep?


Regards,

--- Soji