[Clam-devel] errorhandling

[David García Garzón]

On Wednesday 10 June 2009 16:13:52 dirk.griffioen wrote:
> Hi,
>
> David and me got into a discussion yesterday on howto deal with
> errorconditions in CLAM with respect to reporting 'the right thing' to
> the user. As David mentioned earlier, should we use errorcodes, typed
> exceptions, diagnosis functions or ...
>
> At the moment the codebase has different styles: (boolean) returns,
> asserts (that may register their own handler), throw or even exit():
>
>         if ( jack_port_unregister ( _jackClient, it->jackPort) )
>         {
>             std::cerr << "JACK ERROR: unregistering port " <<
> it->PortName() << std::endl;
>             exit(1);
>         }
>
>
>         if(infile->samplerate() != sampleRate)
>         {
>             std::cout << "All the input audio files have to have the
> same sample rate" << std::endl;
>             exit(-1);
>         }
>
>
> David made the difference between 'programmer exceptions' (violation of
> invariants) which may throw (and possibly terminate) and 'user
> exceptions' - things the user did wrong but should not terminate the
> program; for instance, giving a wrong file may lead to the samplerate
> exit as depicted above.
>
> For 'programmer exceptions' I like the ENFORCE idiom over (clam)assert
> or 'if condition throw error' - as it says what it does, without me
> interpreting code; but opinions may vary here. Either way, 'programmer
> exceptions' are, well, exceptional and should be caught by the main
> catchhandler, logged and the program can terminate if it pleases so.
>
> The big question now becomes, what to tell the user? The samplerate
> message in the above example will not guide the user in choosing the
> correct file.
>
> But maybe if it were more like:
>
>         std::cout << "All the input audio files have to have the same
> sample rate (did you open audiofiles?)" << std::endl;
>
> this would give the user a hint ...
>
> David suggested that lower level (user) exceptions should be handled by
> the higher level, possibly translating them in something more
> meaningfull (David, please correct me if I quote you wrong). The problem
> I see with that is probably will get a large table of 'translations' and
> what to think of the other causes samplerates do not match (corrupted
> file, ...)
>
> I would be all for a simple scheme: state your error and possibly hint
> at a cause, throw and handle it as high as possible.

Let's put an actual use case in clam on why context is important on reporting.

An xml parser detects an error it knows the line and position and what it 
expected. At this level: "unexpected < symbol in line 12 of file a.xml". This 
is the message in an thrown StorageErr that is catch by the (api) user.

The xml parser is used to load the path of a 3d object file whose file name is 
stored in a ProcessingConfig and the load is done in the processing's 
ConcreteConfigure. At this level "XML error while loading object's path: 
unexpected < symbol in line 12 on file a.xml". If the ConcreteConfigure do 
not catch that error is a programmer error, because the upper levels don't 
know that this processing is actually reading XML's.

A configuration error itself is not always an error the (human) user must be 
reported. In fact many processings remain silently unconfigured until we 
configure them. If you start a processing that is unconfigured it is a 
programmer error (assert failed) because the programmer should check first 
that it is configured. If processing user code wants to report the (human) 
user at this level, it should query the configuration error message.

Often that processing is in a network, maybe containing several equal 
processings that may release the same exception. The processing has no 
information of its name in the network but the network knows so when the 
network is to be started i would like to know which are the processings that 
are unconfigured and why. The network provides accessors to collect such 
information.

That network migth be generated in which case, the error is normally a 
programmer error, or might be human user provided in which case the human 
user should be notified. Depending on the interface that could be a dialog or 
a console dump, or maybe we should not report and use a fallback network.

Note that we are using different schemas on reporting, examples are assertions 
for api misusage, api to query status (this is my preferred), exception 
throwing when we want to reach a far caller, like in xml parsing.

Note also that 'api to query status' is not an ErrorCode idiom. We are not 
dealing with errors differently we are just dealing with them.


> Secondly, I have a practical question: what kind of exception should I
> throw? Do I need many types or could I do with 2, one for the progammer
> kind and one for the user kind (please note, I am thinking aloud here).
>
>           class ClamUserException : Err {}
>         class JackException : ClamUserException {}
>
>         if(infile->samplerate() != sampleRate)
>             throw JackException("All the input audio files have to have
> the same sample rate");
>
> where the userexception may be shown to the user at some point ...

This is not for sure as in the previous example. Indeed Assertions migth be 
shown to the user so if the application wants to get reports.

> But I also understood this has been discussed out already - so I'd like
> to know what the findings where in order to implement them to get rid of
> these exit()'s ...

Again, yes, but sure we have needs that are not covered such as in-process and 
backend errors. So, let's concentrate to solve the new problems. If they can 
be solved with the old idioms, ok, if not, let's introduce a new one.


> ENFORCE(infile->samplerate() == sampleRate) << JackException << "All the
> input audio files have to have the same sample rate";

If we can just use c++ without adding more macros:

if (infile->samplerate() == sampleRate)  
	throw JackException() 
		<< "All the input audio files have to have the same sample rate "
		<< "Given: " << infile->samplerate() << " and " samplerate;

Throws, like returns are markers of an (early) exit point of the method.
Macros hide that but are usefull when we want to encode the source file and 
line number.