[jboss-dev-forums] [Design the new POJO MicroContainer] - New Callback and error handling

[adrian@jboss.org]

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! :-)

View the original post : http://www.jboss.com/index.html?module=bb&op=viewtopic&p=4039793#4039793

Reply to the post : http://www.jboss.com/index.html?module=bb&op=posting&mode=reply&p=4039793