New Callback and error handling

[adrian.brock]

I haven't looked at the new Callback code (InstallItems) in any detail yet,
but I noticed immediately that the error handling is insufficient and can lead to inconsistent
behaviour/state.

Take the old implement of incrementState()

try
{
 install();
 updateInternalStateWithSuccess();
}
catch (Throwable t)
{
 error = t;
}
finally
{
 handleErrorHere(); // Invokes uninstall() back to NOT_INSTALLED (then *ERROR*)
}

This code is consistent. It either works and updates the state or if the error
flag has been set it always returns the context to "not installed".

Now we have:

try
{
 install();
 updateInternalStateWithSuccess();
 handleCallbacks(); // NEW Code
}
catch (Throwable t)
{
 error = t;
}
finally
{
 handleErrorHere();
}

The uninstall() includes the unwinding of the new callbacks.

This looks ok? Although it is potentially lazy code. I haven't checked all the codepaths,
to see if it really works.

Better would be:

try
{
 install();
 updateInternalStateWithSuccess();
 try
 {
 handleCallbacks(); // NEW Code
 }
 catch (Throwable t)
 {
 error = t;
 }
 finally
 {
 // Reverse the already succesful install()
 }
}
catch (Throwable t)
{
 error = t;
}
finally
{
 handleErrorHere();
}

Although I think uninstall() is potentially doing this anyway, I'd be more confident
of that fact if the there was a nested try/catch block.

The real problem is the uninstall(). It has two problems.

1) It should be asymmetrical with the install()

// Reverse order of the install()!
removeCallbacksFirst();
context.uninstall();

2) It really does need multiple try/catch blocks.

uninstall() should ATTEMPT to reverse all the changes of install()

Currently it is not doing this.

Previous code:

try
{
 context.uninstall();
}
catch (Throwable t)
{
 // If the callout doesn't work, at least we tried.
 // But continue as it if it did otherwise the state will be inconsistent.
}

New Code:

try
{
 context.uninstall();
 removeCallbacks();
}
catch (Throwable t)
{
 // An error in uninstall() will come here meaning callbacks are not removed!
}

This code reminds me of the horribly poor implementation of a State Machine
in the old ServiceController where I went through and fixed the most obvious
problems with the error handling a few years ago.

Let's not make the same mistakes with the new Microcontainer! :-)

[adrian.brock]

The code really needs to be refactored such that the core install and
before/after methods (the callbacks currently implement only the after pattern)
are handling atomically and consistently.

e.g. In the pseudo code I posted above:

1) An error in one of the callbacks needs to unwind any previously successful callbacks
already invoked (not all of them) and then do the main uninstall()

2) If there is an error in the main install(), we don't even want to try to reverse the callbacks
since they were never invoked in the first place.

You can find an example of this kind of error handling in the MainDeployer/DeployerWrapper.

[adrian.brock]

Finally on the main callback code (not what I've shown above but it has the same
problems!), its questionable whether an error in a callback should stop the main bean
from being installed anyway?

This potentially represents a DOS attack!

@Install
public void dosAttack(DataSource ds)
{
 throw new Error("Nobody can register a DataSource!");
}

An error in a callback should just be logged as a warning about the broken
callback, not stop the original referenced service from being installed.

[alesj]

I haven't looked at the new Callback code (InstallItems) in any detail yet,
but I noticed immediately that the error handling is insufficient and can lead to inconsistent behaviour/state.

Yup, I was waiting for some input, that's why I didn't close the JIRA task yet. ;-)

1) It should be asymmetrical with the install()

// Reverse order of the install()!
removeCallbacksFirst();
context.uninstall();

Yes, I though of that.
But I need the uninstalling context to actually change its state.
For the CollectionCallbackItem to get its collection right on the uninstall phase - not picking up the current uninstalling context.
But I'll try fix this.

Let's not make the same mistakes with the new Microcontainer! :-)

:-)
Agreed.
Will add the necessary error handling.

[adrian.brock]

P.S. When I say "pseudo code" like I did here:
http://www.jboss.com/index.html?module=bb&op=viewtopic&t=105187

It means I've omitted things like error handling, logging and the real implementation
details (which are usually more involved), etc. for clarity.

You should not omit them in the real code! :-)

[alesj]

P.S. When I say "pseudo code" like I did here:
http://www.jboss.com/index.html?module=bb&op=viewtopic&t=105187

It means I've omitted things like error handling, logging and the real implementation
details (which are usually more involved), etc. for clarity.

You should not omit them in the real code! :-)

I already hacked more than 100 classes, so if there was already all the error, logging, ... code commited in as well, I doubt your hard discs could take it. :-)

[alesj]

An error in a callback should just be logged as a warning about the broken
callback, not stop the original referenced service from being installed.

Now all callback invocation error's are suppressed - meaning that context proceeds to next state anyway - if dependencies (cardinality dependency, ...) are satisfied.
I also changed the order on the uninstall to be asymmetric.

Is this now OK?
Or should I add additional error, log, ... handling?