Callback functions, return values, and CORBA exceptions.

[Nat Friedman <nat nat org>]

Hi Miguel,

	As I was writing my ProgressiveDataSink interface tonight, I
noticed something in your implementation of PersistStream that
bothered me.  Because we are setting the precedent for component
authoring, it is important to nail these issues down now, and so I
thought I'd bring it up.

	Let's take one of your interface methods which returns void,
such as load().  In the implementation of the interface, in
gnome-persist-stream.c, you have the following piece of code:

static void
impl_load (PortableServer_Servant servant,
	   GNOME_Stream stream,
	   CORBA_Environment *ev)
{
	GnomeObject *object = gnome_object_from_servant (servant);
	GnomePersistStream *ps = GNOME_PERSIST_STREAM (object);
	int result;
	
	if (ps->load_fn != NULL)
		result = (*ps->load_fn)(ps, stream, ps->closure);
	else {
		GtkObjectClass *oc = GTK_OBJECT (ps)->klass;
		GnomePersistStreamClass *class = GNOME_PERSIST_STREAM_CLASS (oc);
		
		result = (*class->load)(ps, stream);
	}
	if (result != 0){
		g_warning ("FIXME: should report an exception\n");
	}
}

You might remember me commenting last night about your use of the
callback's return value to signal whether or not an error has occured.
If an error has occured, you note in your implementation that a CORBA
exception should be raised.  This is fine as long as this is a
well-defined method-call contract.  The problem is that it is not.
Let me elucidate.

	Let's look at a method in PersistStream which does return a
value, get_size_max().  Your implementation of the interface for this
method looks like this:

static CORBA_long
impl_get_size_max (PortableServer_Servant servant, CORBA_Environment * ev)
{
	GnomeObject *object = gnome_object_from_servant (servant);
	GtkObjectClass *oc = GTK_OBJECT (object)->klass;
	GnomePersistStreamClass *class = GNOME_PERSIST_STREAM_CLASS (oc);

	return ((*class->get_size_max)(GNOME_PERSIST_STREAM (object)));
}

In this case, because the get_size_max() method returns a value -- a
CORBA_long, the maximum size that can be loaded into the PersistStream
-- there is no way for the corresponding callback function to raise an
exception.  This I consider to be a serious design flaw.  This is not
a hard problem to solve, but a well-defined method-call contract must
be devised.

	There are a number of candidate solutions:

	1.  You could pass the CORBA environment to the callback and
	    let it raise the exception itself, manually.  This may be
	    the best option.  Can you think of any reason this might
	    be a bad idea?

	2.  Another solution is to continue to use the callback return
	    value to flag errors (like COM's "hresult"), and to use a
	    function parameter to pass the method's return value back
	    to the interface implementation.  A sample implementation
	    might look like this:

static CORBA_long
impl_get_size_max (PortableServer_Servant servant, CORBA_Environment * ev)
{
	GnomeObject *object = gnome_object_from_servant (servant);
	GtkObjectClass *oc = GTK_OBJECT (object)->klass;
	GnomePersistStreamClass *class = GNOME_PERSIST_STREAM_CLASS (oc);

	CORBA_long corba_retval;
	int result;

	result =  ((*class->get_size_max)(GNOME_PERSIST_STREAM (object),
					  &corba_retval));

	if (result != 0)
	{
		/* Raise an exception */
	}

	return corba_retval;
}

	    This could be considered a little bit gross, but it would
	    suffice as well.  What's imperative is that we replace the
	    current protocol with something that works.

Best wishes,
Nat