BUG: Clean up xout before rethrow

[kaspermarstal]

Elastix did not call AfterRegistrationBase before throwing exception and consequently did not clean up xout. This resulted in undefined behaviour and more or less random segfaults if the ElastixFilter was called again after an exception was thrown. This would happen even if the filter was completely destroyed and a new filter was instantiated because xout is a global static object. This possibly fixes SuperElastix/SimpleElastix#123 and an issue previously reported by @coertmetz.

The PR needs more testing and thorough review before merge. In addition, we should review exception handling elsewhere - e.g. for transformix.

NOTE: This does not change the fact that elastix is not thread safe which might still be an issue (also reported by @coertmetz). The logger will most likely have to be completely refactored into a member variable on e.g. ElastixMain for the library interface to become thread safe.

[coertmetz]

This seems to make a lot of sense. I do not have enough knowledge about AfterRegistrationBase to get an idea of possible side-effects however.

[mstaring]

so AfterRegistrationBase() currently contains one line:

xl::xout.RemoveTargetCell( "iteration" );

I am not sure why this cell should be removed before throwing, but your explanation makes intuitive sense to me. At the least I see no harm in removing it, as this particular exception completely stops the registration altogether and (safely) terminates elastix.

@kaspermarstal Did you do a test with and without this change and indeed observed segfaults going away?
Is this test something that Coert could repeat?

[kaspermarstal]

Yes, @woutdenolf made a minimum working example with data in SuperElastix/SimpleElastix#123. You can get it there @coertmetz.

Yes, this patch fixes the problem on my machine -- I have not observed any segfault with the patch applied. However I have not been able to find out exactly what is going wrong when AfterRegistrationBase is not called. Sometimes it segfaults, sometimes it doesn't.

I can only assume that the logger somehow ends up in an inconsistent state if "iteration" is not removed -- but why or how I do not know.

[mstaring]

Ok, so if it also works at Coert's side, then please go ahead with merging. In case Coert doesn't have time, go ahead with merging also.

[kaspermarstal]

Is it something you plan to test @coertmetz?

[stefanklein]

The iteration info output is of type xoutrow. And this is the only occasion where xoutrow is used. So maybe the underlying bug is in that class (could be, as it is the most complicated xout type). Maybe the destructor of xoutrow does not properly clean up itself for example.
To solve it, I think it might be better to call directly:
xl::xout.RemoveTargetCell( "iteration" );
instead of calling AfterRegistrationBase(). You in principle don't know what "AfterRegistrationBase" does more (currently nothing), which should not be done after an error has been thrown. Also, it would be unclear in the code why you call this function (since it is actually an indirect way to clean up xout; so, better to directly do that I think).

[coertmetz]

Hi @stefanklein,

This is more or less what we now do at Quantib side as a work around. We catch the exceptions of elastix. In the catch block we do:

if (xoutlibrary::xout_valid()) {
    xoutlibrary::xout.RemoveTargetCell("iteration");
}

For this to work we added the xout_valid to the xoutlibrary. This method just checks if local_xout is not null:

bool xout_valid() {
  return local_xout != 0;
}

But of course, this should in principle be fixed within elastix.

[ascourtas]

to get these changes, would I need to rebuild the project, or is it sufficient to just pull down the updated repo?