Capsule, Unencapsulate : Fix cancellation handling #5792
Conversation
We weren't omitting the canceller when copying the context in the Capsule constructor. This meant the Capsule could contain a dangling pointer to a long-since destructed Canceller, which among other things could cause `IECore::Cancelled` to be thrown when unencapsulating or rendering the capsule. Even if the Canceller wasn't dangling, it still wasn't suitable to be stored, because the Capsule is stored in Gaffer's cache and could be used subsequently by a client for which the original canceller doesn't apply. A similar bug existed for the Unencapsulate node, which needs to ensure the _current_ Canceller is used when evaluating the Capsule contents. We do this in CapsuleScope by combining the current canceller with the Context stored in the Capsule, via an EditableScope. There was one limited scenario in which the first bug could actually have worked in our favour, and which isn't matched by the fix. When rendering a freshly-minted capsule, the canceller might have still been valid, allowing cancellation of a capsule expansion. This wouldn't have applied to any of our current rendering mechanisms though; neither batch nor interative renders use cancellation, and the Viewer doesn't expand capsules. This does point out a limitation of Capsules though (and Procedurals more generally) - we need to add a mechanism to pass cancellers to the `render()` method.
| context["test"] = 1 | ||
|
|
||
| capsule = GafferScene.Capsule( sphere["out"], "/", context, hash, sphere["out"].bound( "/" ) ) | ||
| self.assertIsNone( capsule.context().canceller() ) |
There was a problem hiding this comment.
This test looks a bit weird - it goes to the effort of computing an accurate hash, and puts a "test" entry in the context so we can distinguish it from a default context ... but then never tests any of these things?
There was a problem hiding this comment.
it goes to the effort of computing an accurate hash
The former I just copied from another test, not so much for testing purposes, but so that the test demonstrates that the Capsule maker is responsible for making a suitable hash. I think this is OK?
and puts a "test" entry in the context so we can distinguish it from a default context
This was me completing botching things. I had intended to use test to check that the correct context got through (just not with the canceller). And of course that context should be scoped before the hash calls.
doesn't need to prevent merging this if we're trying to get a release out.
I'm signing off now, and I agree this doesn't need to block the release. But I'll make a PR first thing tomorrow to improve the test...
There was a problem hiding this comment.
Thanks for catching that...
We weren't omitting the canceller when copying the context in the Capsule constructor. This meant the Capsule could contain a dangling pointer to a long-since destructed Canceller, which among other things could cause
IECore::Cancelledto be thrown when unencapsulating or rendering the capsule. Even if the Canceller wasn't dangling, it still wasn't suitable to be stored, because the Capsule is stored in Gaffer's cache and could be used subsequently by a client for which the original canceller doesn't apply.A similar bug existed for the Unencapsulate node, which needs to ensure the current Canceller is used when evaluating the Capsule contents. We do this in CapsuleScope by combining the current canceller with the Context stored in the Capsule, via an EditableScope.
There was one limited scenario in which the first bug could actually have worked in our favour, and which isn't matched by the fix. When rendering a freshly-minted capsule, the canceller might have still been valid, allowing cancellation of a capsule expansion. This wouldn't have applied to any of our current rendering mechanisms though; neither batch nor interative renders use cancellation, and the Viewer doesn't expand capsules. This does point out a limitation of Capsules though (and Procedurals more generally) - we need to add a mechanism to pass cancellers to the
render()method.