Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ImageBuf+IOProxy vs ImageCache subtle fixes #3666

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Nov 13, 2022

I uncovered a very subtle problem that can happen with the interaction between ImageBuf, using an IOProxy, and the underlying ImageCache. Remember that when using an IOProxy, it is critical that the lifetime of the proxy outlasts any ImageInput, ImageBuf, or ImageCache that was told about it.

Consider the following code:

IOProxy proxy(...);   // temporary proxy in this local scope
ImageBuf A ("foo.exr", 0, 0, proxy);

We've set up an ImageBuf that will read from an IOProxy. Behind the scenes, it made an ImageCache entry that remembers the IOProxy, as it has to, since it needs to use that proxy for subsequent reads. Now we read the image, and we use the force=true parameter to force an immediate read of the file:

A.read (0, 0, true);

It reads from the proxy. But the IB and the underlying entry in the IC still know about the proxy. But in the user's mind, they have fully read the image, will never read from it again, so of course (they think) it is safe to close or destroy the proxy, knowing that they made it last past its use:

proxy.close();

But -- oops! -- the cache entry still has a pointer to the proxy, and even though it won't need to read from it again, it still will access it, close it, etc., but the user's proxy may by that point be out of scope.

This is technically not a "bug" in the sense that it is the user who violated the contract by closing or destroying the proxy before its last use by anything they passed it to. But in context, it's very easy to be confused by this -- the user thinks that the only observer of the proxy is the ImageBuf, and they're done using the ImageBuf at that point, but because they don't explicitly mention an ImageCache, they don't realize that the ImageBuf passed the proxy reference along to the underlying IC that could be backing any ImageBuf.

It's the user's fault in some sense, but let's just say that we have given them an "attractive nuisance."

That's a long bit of background explanation, but I wanted to force myself to explain it clearly and make sure it was spelled out in the commit comments. Now here is the fix:

  • When IB::read does an immediate forced read involving an IOProxy, after the read is complete, tell the cache to invalidate the entry for the file so that it no longer retains a pointer to the proxy.

  • When an IB itself is destroyed or cleared, if it used an IOProxy, also invalidate the IC entry for that file.

  • Unrelated cleanup: avoid a redundent invalidate call in init_spec().

I uncovered a very subtle problem that can happen with the interaction
between ImageBuf, using an IOProxy, and the underlying ImageCache.
Remember that when using an IOProxy, it is critical that the lifetime
of the proxy outlasts any ImageInput, ImageBuf, or ImageCache that was
told about it.

Consider the following code:

    IOProxy proxy(...);   // temporary proxy in this local scope
    ImageBuf A ("foo.exr", 0, 0, proxy);

We've set up an ImageBuf that will read from an IOProxy.  Behind the
scenes, it made an ImageCache entry that remembers the IOProxy, as it
has to, since it needs to use that proxy for subsequent reads.
Now we read the image, and we use the `force=true` parameter to
force an immediate read of the file:

    A.read (0, 0, true);

It reads from the proxy. But the IB and the underlying entry in the IC
still know about the proxy. But in the user's mind, they have fully
read the image, will never read from it again, so of course (they
think) it is safe to close or destroy the proxy, knowing that they
made it last past its use:

    proxy.close();

But -- oops! -- the cache entry still has a pointer to the proxy, and
even though it won't need to read from it again, it still will access
it, close it, etc., but the user's proxy may by that point be out of
scope.

This is *technically* not a "bug" in the sense that it is the user who
violated the contract by closing or destroying the proxy before its
last use by anything they passed it to. But in context, it's very easy
to be confused by this -- the user thinks that the only observer of
the proxy is the ImageBuf, and they're done using the ImageBuf at that
point, but because they don't explicitly mention an ImageCache, they
don't realize that the ImageBuf passed the proxy reference along to
the underlying IC that could be backing any ImageBuf.

It's the user's fault in some sense, but let's just say that we have
given them an "attractive nuisance."

That's a long bit of background explanation, but I wanted to force
myself to explain it clearly and make sure it was spelled out in the
commit comments. Now here is the fix:

* When IB::read does an immediate forced read involving an IOProxy,
  after the read is complete, tell the cache to invalidate the entry
  for the file so that it no longer retains a pointer to the proxy.

* When an IB itself is destroyed or cleared, if it used an IOProxy,
  also invalidate the IC entry for that file.

* Unrelated cleanup: avoid a redundant invalidate call in init_spec().
@lgritz lgritz merged commit d7ecbbb into AcademySoftwareFoundation:master Nov 15, 2022
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Nov 16, 2022
…n#3666)

I uncovered a very subtle problem that can happen with the interaction
between ImageBuf, using an IOProxy, and the underlying ImageCache.
Remember that when using an IOProxy, it is critical that the lifetime
of the proxy outlasts any ImageInput, ImageBuf, or ImageCache that was
told about it.

Consider the following code:

    IOProxy proxy(...);   // temporary proxy in this local scope
    ImageBuf A ("foo.exr", 0, 0, proxy);

We've set up an ImageBuf that will read from an IOProxy.  Behind the
scenes, it made an ImageCache entry that remembers the IOProxy, as it
has to, since it needs to use that proxy for subsequent reads.
Now we read the image, and we use the `force=true` parameter to
force an immediate read of the file:

    A.read (0, 0, true);

It reads from the proxy. But the IB and the underlying entry in the IC
still know about the proxy. But in the user's mind, they have fully
read the image, will never read from it again, so of course (they
think) it is safe to close or destroy the proxy, knowing that they
made it last past its use:

    proxy.close();

But -- oops! -- the cache entry still has a pointer to the proxy, and
even though it won't need to read from it again, it still will access
it, close it, etc., but the user's proxy may by that point be out of
scope.

This is *technically* not a "bug" in the sense that it is the user who
violated the contract by closing or destroying the proxy before its
last use by anything they passed it to. But in context, it's very easy
to be confused by this -- the user thinks that the only observer of
the proxy is the ImageBuf, and they're done using the ImageBuf at that
point, but because they don't explicitly mention an ImageCache, they
don't realize that the ImageBuf passed the proxy reference along to
the underlying IC that could be backing any ImageBuf.

It's the user's fault in some sense, but let's just say that we have
given them an "attractive nuisance."

That's a long bit of background explanation, but I wanted to force
myself to explain it clearly and make sure it was spelled out in the
commit comments. Now here is the fix:

* When IB::read does an immediate forced read involving an IOProxy,
  after the read is complete, tell the cache to invalidate the entry
  for the file so that it no longer retains a pointer to the proxy.

* When an IB itself is destroyed or cleared, if it used an IOProxy,
  also invalidate the IC entry for that file.

* Unrelated cleanup: avoid a redundant invalidate call in init_spec().
@lgritz lgritz deleted the lg-ib branch December 4, 2022 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant