On May 12, 2018, at 3:25 AM, Gregor Mückl <[email protected]> wrote:
> I need to reply to myself here because I finally found a good explanation
> of why this sometimes works (and I turned out to be lucky) and sometimes
> fails miserably. It's on Raymond Chen's blog of all places:
>
> https://blogs.msdn.microsoft.com/oldnewthing/20161209-00/?p=94905
Thanks for the link!
It's insidious. Not only does it "sometimes work / sometimes fail" on Windows,
but it will *always* work for developers who work on Linux or OSX, only to find
that when other people use their code on Windows, it will misbehave.
> So OIIO really must change in order to cope with all the different
> possibilities how program heaps can be structured on Windows.
Yes, though that's not exactly how I'd characterize the main point of this
patch.
We've actually been aware of this issue all along, and that's why there was
always an ImageInput::destroy() method -- because if an app calls 'delete'
itself, the free will be on the app side but the allocation was in
libOpenImageIO (different side of the dll boundary, so potentially different
heap). But it had always been the case that an app programmer could easily
forget to do this, call delete instead of II::destroy(), and make trouble. And
in the process of doing this work (but not the impetus for it) I realized that
destroy() would still fail for *custom* ImageInputs, which might not be
implemented in OpenImageIO.dll (where destroy lives).
Fully managed pointers with deleters (that execute the free in the same dll
that did the allocation) is a more robust fix.
The real impetus for this change, though, was just to fully clean up all the
potential errors that could easily happen when we return a raw pointer:
App never destroys the pointer (leak) -- can no longer happen.
App accidentally calls delete instead of ImageInput::destroy() (it will
silently work on Linux/OSX, but subtly break on Windows because the free can
happen on the wrong side of the dll boundary) -- can no longer happen.
App accidentally calls free() instead of delete/destroy (ouch!) -- can no
longer happen.
App function that created the ImageInput passes it to another subroutine... is
it transferring ownership? Who is responsible for freeing? Now it's clear -- if
the unique_ptr is moved to you, you are the new owner, but if you just get a
raw pointer, somebody else owns the resource and you are just a visitor.
It also just simplifies code and documentation. Something that previously
looked like
ImageInput *in = ImageInput::open ("foo.tif");
if (! in) {
return;
}
if (in->spec makes it clear this isn't the image you want) {
ImageInput::destroy (in); // <---------
return;
}
ok = in->read_image(...);
if (! ok) {
... more error processing ..
ImageInput::destroy (in); // <---------
return;
}
my_pixel_processing (...);
ImageInput::destroy (in); // <---------
It's exhausting to have to find all the places where that raw pointer might be
dangling and be sure to destroy it. The new code equivalent is:
auto in = ImageInput::open ("foo.tif");
if (! in)
return;
if (in->spec makes it clear this isn't the image you want)
return;
ok = in->read_image(...);
if (! ok) {
... more error processing ..
return;
}
my_pixel_processing (...);
So much cleaner, shorter and clearer examples for the user manual, less chance
of bugs.
And did you spot the bug still in the original? What happens if
my_pixel_processing() throws an exception that doesn't get caught until a
couple levels up the call stack? That ImageInput will never be freed at all.
But it will the new way, as soon as execution leaves the scope where 'in' was
declared.
--
Larry Gritz
[email protected]
_______________________________________________
Oiio-dev mailing list
[email protected]
http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
