New Callback and error handling
adrian.brock Apr 23, 2007 7:26 AMI 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! :-)