[Mono-dev] New error handling framework for mono

[Paolo Molaro]

On 08/20/09 Rodrigo Kumpera wrote:
> > I don't like MONO_ERROR_ON_SIGNAL_CONTEXT: most code is not going to
> > know in what mode it was called in. Simply always use the message
> > buffer.
> 
> 
> I changed this to have a MONO_ERROR_STORE_FULL_MESSAGE flag. My reasoning
> behind this is
> that some messages are exceptionally large, specially when it does include
> the assembly path. Look
> at loader.c:330, just the format string is more than 320 chars long and I
> don't want to embed it inside
> the MonoError machinery.
> 
> Now about MONO_ERROR_ON_SIGNAL_CONTEXT, it made sense to me when the caller
> did init
> MonoError . Now that this change, it has to change.
> 
> I've added a MONO_ERROR_DONT_COPY_STRINGS flag but, honestly, I don't think
> it is that
> helpfull and it doesn't help at all into make it safe for use under signal
> context.

I'm not happy with both the MONO_ERROR_STORE_FULL_MESSAGE and
MONO_ERROR_DONT_COPY_STRINGS flags you introduced.
For the message this could be handled automatically: if it doesn't fit
into the buffer you can malloc it at set time, the code that initializes the
MonoError has no way to know if the message will fit or not.
Similarly, the initializing code can't know if any of the strings need
to be strduped because it's far away from the code that sets it.
This should be decided at the callsite, either passing a flag or calling
a separate function that does the dup.

A small note on overlong messages. I think we should have a policy like
this:
*) error messages should be short sentences
*) any non-essential description of the error and suggestions for
workarounds should go into either or both the logging mechanism and the
documentation (wiki etc.)

> But what should be done when we get an allocation failure on the
> mono_exception_set_*
> function? Should we just convert such error into an OOM, abort due to a
> double-fault or
> silently ignore those arguments (and print something on console).

It is tricky. In some cases we want to communicate an OOM exception to
the managed code. In some others we can't: we need to give the caller
code a chance to handle the error and not crash.
Using a flag here can be helpful: something like MONO_ERROR_INCOMPLETE
to signal that the full info was not stored for whatever reason.
That should about strike a good balance between being robust and keeping
track of as much info as we can. Aborting here is not an option, IMHO.

> --- /dev/null
> +++ b/mono/utils/mono-error.c
> @@ -0,0 +1,385 @@
> +/*
> + * mono-error.c: Error handling code
> + *
> + * Authors:
> + *	Rodrigo Kumpera    (rkumpera at novell.com)
> + * Copyright 2009 Novell, Inc (http://www.novell.com)
> + */
> +#include <glib.h>
> +
> +#include "mono-error.h"
> +#include "mono-error-internals.h"
> +
> +#include <mono/metadata/exception.h>
> +#include <mono/metadata/object-internals.h>
> +#include <mono/metadata/debug-helpers.h>
> +
> +#define mono_error_get_message(E) ((E)->full_message? (E)->full_message : (E)->message)
> +
> +#define set_error_message() do { \
> +	va_start (args, msg_format); \
> +	if (error->flags & MONO_ERROR_STORE_FULL_MESSAGE) \
> +		error->full_message = g_strdup_vprintf (msg_format, args); \
> +	else\
> +		g_vsnprintf (error->message, 128, msg_format, args); \

This should be sizeof (error->message) instead of the magic number 128.

> +	va_end (args); \
> +} while (0)
> +
> +void
> +mono_error_init_flags (MonoError *oerror, unsigned short flags)
> +{
> +	MonoErrorInternal *error = (MonoErrorInternal*)oerror;
> +	g_assert (sizeof (MonoError) == sizeof (MonoErrorInternal));
> +
> +	error->error_code = MONO_ERROR_NONE;
> +	error->flags = flags;
> +	error->type_name = error->assembly_name = error->member_name = error->full_message = error->exception_name_space = error->exception_name = NULL;
> +	error->klass = NULL;
> +	error->message [0] = 0;

Performance tip: until error_code is MONO_ERROR_NONE there is no need to
initialize all the other fields (except flags). This speeds up the
common path. Of course you need to check error_code in the cleanup
function.

> +void
> +mono_error_set_corlib_exception (MonoError *oerror, const char *name_space, const char *name)
> +{
> +	MonoErrorInternal *error = (MonoErrorInternal*)oerror;
> +
> +	if (error->flags & MONO_ERROR_DONT_COPY_STRINGS) {
> +		error->exception_name_space = name_space;
> +		error->exception_name = name;
> +	} else {
> +		error->exception_name_space = g_strdup (name_space);
> +		error->exception_name = g_strdup (name);
> +	}

This should set error_code as well (maybe have it passed as an argument).

> +void
> +mono_error_set_out_of_memory (MonoError *oerror, const char *msg_format, ...)
> +{
> +	MonoErrorInternal *error = (MonoErrorInternal*)oerror;
> +	va_list args;
> +
> +	error->error_code = MONO_ERROR_OUT_OF_MEMORY;
> +	error->flags &= ~MONO_ERROR_STORE_FULL_MESSAGE; /*Can't really allocate memory under OOM*/
> +	va_start (args, msg_format);
> +	g_vsnprintf (error->message, 128, msg_format, args);

Magic number here again.

Thanks!

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better