[Clam-devel] errorhandling

[dirk.griffioen]

Just a small reply :)
> 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.
>
>
>   
Ok, so if I read this correctly you're saying 'handle as needed'.

Then, like we said yesterday - it may be best to go over concrete 
situations and decide what needs to be done ...
>> 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.
>
>   
If I may, I would like to advocate a more clear position, a bit after 
the 'guru of the week' article I mentioned earlier.
- exit: may not be used
- asserts: used locally (when the caller is in control of parameters, 
which should be correct to the callee)
- exceptions: to pass errors upwards
- return codes: for instance on api level (say after a catch internally)
>   
>> 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.
>
>
>
>   
Ok - that's a valid remark; but the reason I would choose the macro over 
the lines that clearly states the invariant is violated, enforce 
enforces invariants, if checks conditions. These are different things.

But I guess we will not see easily agree on this one :)

Dirk
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.clam-project.org/pipermail/clam-devel-clam-project.org/attachments/20090611/5773b862/attachment.html>