[Mono-dev] New error handling framework for mono

[Paolo Molaro]

On 08/18/09 Rodrigo Kumpera wrote:
> > Still open is how this would be integrated on 2.6

I think we should keep this internal for 2.6 and expose it in 2.8 wth
the other API changes, once we have gained experience with the new API.

> and if functions should
> > error out if passed an already set error object.

I think as a general rule the called function should set the
error code to OK. We already have this pattern (like in
mono_image_open ()) and we should keep it.
Once we have called a function that can and does result in an error, it
is pointless to continue execution.

A comment about the use of TLS data that was raised in the thread:
*) we use that pattern already in mono and it has been proved to be
a huge source of bugs
*) it introduces issues when an additional error happens while handling
the first one (info is either overwritten and lost or you have to
manually keep track of it)

The first one is reason enough to avoid doing the same mistake twice.

> --- a/mono/metadata/Makefile.am
> +++ b/mono/metadata/Makefile.am
> @@ -160,6 +160,9 @@ libmonoruntime_la_SOURCES = \
>  	threadpool-internals.h	\
>  	verify.c		\
>  	verify-internals.h	\
> +	mono-error.c	\
> +	mono-error.h	\
> +	mono-error-internal.h	\

This should be in mono/utils, so we can use it also in the code there.

> --- /dev/null
> +++ b/mono/metadata/mono-error-internals.h
> +void
> +mono_error_set_type_load (MonoError *error, MonoClass *klass, char *msg_format, ...) MONO_INTERNAL;
> +
> +void
> +mono_error_set_type_load2 (MonoError *error, char *type_name, char *assembly_name, char *msg_format, ...) MONO_INTERNAL;

"load2" is ugly, use something like mono_error_set_type_load_name,
instead. The other one could be called mono_error_set_type_load_class to
keep more symmetry in the naming.

> +MonoException*
> +mono_error_prepare_exception (MonoError *error, MonoDomain *domain) MONO_INTERNAL;

This is one function where you could actually start using the new
mechanism, adding a MonoError *error_out as the last argument.

> --- /dev/null
> +++ b/mono/metadata/mono-error.c
> @@ -0,0 +1,253 @@
> +
> +#define mono_error_get_message(E) (((E)->flags & MONO_ERROR_ON_SIGNAL_CONTEXT) ? (E)->message : (E)->full_message)

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.

> +void
> +mono_error_init (MonoError *error, unsigned short flags)

The common usage will be without any flags, so I would have the simpler
	void mono_error_init (MonoError *error)
and
	void mono_error_init_flags (MonoError *error, unsigned short flags)
in the case we actually want to set the non-default flags.

> +void
> +mono_error_cleanup (MonoError *error)
> +{
> +	if (error->flags & MONO_ERROR_ON_SIGNAL_CONTEXT) //no memory was allocated
> +		return;

A flag that tells the code the strings are to be freed is more
general-purpouse than MONO_ERROR_ON_SIGNAL_CONTEXT, IMHO.

> +void
> +mono_error_set_assembly_name (MonoError *error, char *assembly_name)

It's a good idea to have the string arguments be const char*.

> +void
> +mono_error_set_assembly_load (MonoError *error, char *assembly_name, char *msg_format, ...)
> +{
> +	va_list args;
> +	error->error_code = MONO_EXCEPTION_FILE_NOT_FOUND;

File not found is not the only failure case for an assembly load, we
should be more flexible here. Think of a bad image, for example, but
also of an out of memory issue.

> --- /dev/null
> +++ b/mono/metadata/mono-error.h
> @@ -0,0 +1,35 @@
> +#ifndef __MONO_ERROR_H__
> +#define __MONO_ERROR_H__
> +
> +#include <mono/metadata/class.h>
> +#include <mono/metadata/metadata.h>
> +
> +/*Don't allocate memory or call signal unsafe functions*/
> +#define MONO_ERROR_ON_SIGNAL_CONTEXT 0x0001

We should use an enum for the flags.

> +typedef struct {
> +	unsigned short error_code; //MONO_EXCEPTION_*
> +    unsigned short flags; //MONO_ERROR_*
> +
> +	char *type_name;
> +	char *assembly_name;
> +	char *member_name;
> +	MonoClass *klass;
> +	char *full_message;
> +	gpointer padding [4];
> +    char message [128];

The field names should be clearly marked as "don't touch", except
maybe error_code. Names like hidden_1, hidden_2 etch should work.
Also use void* as the type.
In the internal code at the top of the file you can then do:

#define type_name ((char*)hidden_1)

to keep the code readable. It's kind of an hack.
An alternative is to have a separate struct with the correct types and
names and cast to it on public function entry.

Thanks, looking forward an updated patch!

lupus

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