[Insight-developers] Broken backwards compatibility -- results of 2 days debugging

[Daniel Blezek]

Hi Bill,

  In principle, the code should check for NULL... but... the code just
allocated an output image, and set it using SetNthOutput, any reasonable
developer would expect to get back the value he just set.  If the
SetNthInput only allowed TOutputImage, then the compiler would have caught
this, but because SetNthInput allowed the code to stuff an image of an
unexpected type into the m_Outputs array, Luca was very reasonable in
expecting to get it back.

  Of course the "fix" for this is to call ProcessObject::GetOutput(2),
effectively circumventing the type correctness of the class.

  I don't know what the right thing to do is in this case...

-dan


On 8/25/09 11:04 AM, "Bill Lorensen" <bill.lorensen at gmail.com> wrote:

> At a minimum that fix should have included a warning if the dynamic
> cast failed. Also, Lucas' code should be fixed to check for the NULL.
> 
> 
> Bill
> 
> On Tue, Aug 25, 2009 at 10:42 AM, Daniel Blezek<Blezek.Daniel at mayo.edu> wrote:
>> Hi  all,
>> 
>>   I had integrated Luca Antiga¹s vesselness code into our application a
>> while ago.  Fairly recently, it has been breaking on one of our platforms,
>> the only one building with the CVS head version of Insight.  I finally
>> tracked the crash down to this sequence.
>> 
>> Luca¹s code creates output images of different types and caches them in the
>> outputs:
>>   typename HessianImageType::Pointer hessianImage = HessianImageType::New();
>>   this->ProcessObject::SetNthOutput(2,hessianImage.GetPointer());
>> 
>> Later, he grabs the output and allocates the buffers:
>>     this->GetOutput(2)->SetBufferedRegion(this->GetOutput(2)->GetRequestedReg
>> ion());
>>     this->GetOutput(2)->Allocate();
>> 
>> This code crashes due to this change:
>> revision 1.64
>> date: 2009-03-10 09:38:35 -0500;  author: blowekamp;  state: Exp;  lines:
>> +24 -10;  commitid: 0s3FC2A2iqP1NuFt;
>> BUG: 8681 ImageSource lacks type safety for multiple outputs of different
>> types. Changed static_cast to dynamic_cast in GetOutput(unsigned int), so
>> that we don't return an invalid pointer. GraftNthOutput now uses type safe
>> ProcessObject::GetOutput. AllocateOutputs previously would segfault under
>> these conditions. Now it safely casts to ImageBase and calls Allocate
>> methods.
>> 
>> which made this code use dynamic_cast, rather than static_cast:
>> 
>>   return dynamic_cast<TOutputImage*>
>>     (this->ProcessObject::GetOutput(idx));
>> 
>> 
>> So now, my GetOutput(2) returns null (cause it¹s not a TOutputImage) and
>> everything crashes.
>> 
>> This was highly frustrating to me.  Yes I know this is a bug, and I know it
>> should be fixed, but code which depended on this behavior (Luca¹s first
>> implementation) is now broken.  Does this constitute a break of backwards
>> compatibility?
>> 
>> I propose at least a warning if the static_cast and dynamic_cast return
>> different pointers.  This would have saved me two days of deep debugging and
>> wondering why the same code ran fine on an older release version of ITK.
>> 
>> Best regards,
>> -dan
>> 
>> 
>> 
>> --
>> Daniel Blezek, PhD
>> Medical Imaging Informatics Innovation Center
>> 
>> P 127 or (77) 8 8886
>> T 507 538 8886
>> E blezek.daniel at mayo.edu
>> 
>> Mayo Clinic
>> 200 First St. S.W.
>> Harwick SL-44
>> Rochester, MN 55905
>> mayoclinic.org
>> 
>> 
>> _______________________________________________
>> Powered by www.kitware.com
>> 
>> Visit other Kitware open-source projects at
>> http://www.kitware.com/opensource/opensource.html
>> 
>> Please keep messages on-topic and check the ITK FAQ at:
>> http://www.itk.org/Wiki/ITK_FAQ
>> 
>> Follow this link to subscribe/unsubscribe:
>> http://www.itk.org/mailman/listinfo/insight-developers
>> 
>> 

-- 
Daniel Blezek, PhD
Medical Imaging Informatics Innovation Center

P 127 or (77) 8 8886
T 507 538 8886
E blezek.daniel at mayo.edu

Mayo Clinic
200 First St. S.W.
Harwick SL-44
Rochester, MN 55905
mayoclinic.org