2c7b820bc8
A very common pattern in our synthetic child providers was to make the child ValueObject using ValueObjectConstResult::Create or some form of the static ValueObject::CreateValueObjectFrom*** methods, and store and hand that out as the child. Doing that creates a "root" ValueObject whose lifecycle is not linked to the lifecycle of the ValueObject it is a child of. And that means it is possible that either the child or the parent could have gotten destroyed when the other ValueObject gets asked a question about it. For the most part this doesn't happen because there are usually enough other shared pointer references binding the two to keep both sides alive. But we have gotten a small but steady stream of reports for years now of crashes where a ValueObject accesses its ClusterManager but that has already been deleted. I've never been able to find a reproducible case of this, but one plausible cause is that we are violating the contract that "all the children of a ValueObject have coterminous lifespans, enforced by the ClusterManager". So it is unsurprising that they might sometimes not stay alive as long as they should. This patch addresses that by providing a way to use these static create methods but passing in the ClusterManager to be used, and adds or modifies where extant the CreateChildValueObjectFrom*** methods to pass in the parent manager. This patch is not complete. It turns out that routing the ClusterManager from CreateValueObjectFromExpression all the way to where the expression result VO is made is quite intrusive, and would have made this already quite large (though largly mechanical) patch become unweildy. Since I am mostly concerned about synthetic child providers, and we discourage using expressions for them, I think that's an acceptable separation. I also added a test where the ValueObjectSynthetic hands out its children, checking whether the child being handed out is in fact owned by the parent handing it out. It is controlled by a setting (target.check-vo-ownership) which is currently off by default - since I can't provide a way to do this correctly for expressions yet. But I turn it on in the testsuite. That's the real testing strategy for this patch, since its goal is to ensure that all the children we hand out are managed by their parents. I didn't put an equivalent check in ValueObjectVariable because that system really doesn't allow a way to do this incorrectly. I also fixed some of the tests that were making children using CreateValueObjectFromExpression to make their children with CreateValueObjectFromData instead. I could have opted the tests out of the check, but using the expression parser to do this work is not best practice, so I didn't want to leave bad examples around. I left one case that does do it with expressions, however, so we still have a test for that not-recommended but people do it anyway path.
16 KiB
16 KiB