Fusion
Fusion copied to clipboard
dphfox
applyInstanceProps can become the sole referencer of accessible instances, leading to lost references
When using Parent with applyInstanceProps, it's possible to lose all references to the instance in user code while still being able to execute changes on the instance indirectly via state objects, or observe changes on the instance indirectly via events or out props. In this scenario, applyInstanceProps becomes the sole referencer of the instance.
Normally, when applyInstanceProps becomes the sole references of the instance, we depend on the instance's accessibility to determine whether to continue holding the reference or drop it. The criteria are specified in isAccessible.lua, utilised by the reference-holding mechanism in semiWeakRef.lua.
There seems to be a nasty edge case where, in spite of the instance being accessible, the reference is lost by applyInstanceProps due to it being held too weakly. This typically occurs when parenting an instance to nil using bound state on the Parent property, then attempting to bring the instance back into the data model after an extended period of time. While in nil, the code may lose the reference to the instance, even though it is live.
This will require a lot of design to get right.
Repro: https://cdn.discordapp.com/attachments/895437663040077834/936512260480843816/Repro.rbxl
I think the issue here is fundamental to how Roblox is designed - the nil parent does not have a single responsibility. You cannot tell whether an instance in nil is ready to be destroyed or is only there temporarily.
Fusion's current interpretation is that instances in nil are only ready to be destroyed when the user no longer holds their own references to them, which seems sensible at first glance but actually is not a sound assumption. Bound properties provide a mechanism for effecting change on an instance without having to maintain a reference in the user's code, which is the original source of this issue.
It's interesting to consider that Fusion makes a similar-ish assumption for state objects. State objects are considered ready for cleanup when they have no dependents. This makes more sense since you need a reference to a state object to add it as a dependency, thus a state object with no dependents and no reference will never have new dependents, and so will never again have an influence on the reactive graph, making it safe to destroy. This is therefore a sound assumption as long as the state objects don't have external effects (which is why Observer does not clean itself up).
To resolve this, we either need to figure out some metric for evaluating whether a state object is in use, or concede that we shouldn't mix destroyed and active instances under the same parent.
I'm currently leaning towards the latter, by the way; it's the prevailing wind with Roblox development nowadays, and would lead to a simpler system.
The decision would then become 'which parent should represent destroyed instances?'. Whichever parent we use would then no longer be allowed for regular use.
The obvious choice here is to use nil as the destroyed parent, though it's not our only option. I briefly played with the idea of Fusion providing a special parent where destroyed instances can go, but it's a pretty terrible idea and inconsistent with basically every single Roblox coding practice in history. I don't think that's an idea worth pursuing any further.
This now raises further questions about how Fusion operates. When we unparent children via the [Children] key, for example, we would effectively be marking them for destruction. However, the intent of that behaviour was to mark the children as 'orphaned', since we didn't want them parented here, but have no other place to move them to right now. We could rework the theory here to make child destruction the 'proper' behaviour, but would this be sensible in all cases? What about if we're parenting children not managed by Fusion? Is that a reasonable thing to do, or should we disallow these uses in favour of using Parent? Maybe Fusion should provide a 'foster parent' rather than parenting to nil?
Making nil parent meaningful would therefore require a complete re-evaluation of every place in the Fusion library where we parent to nil, perhaps requiring some deep thinking about the theory which underpins these features.
But hey! On the bright side, that gives us a nice declarative way to destroy instances - use a Computed for Parent and return nil to destroy 😛
(though maybe we should error for that instead, just in case someone tries it as a way of temporarily storing instances. This bug report proves it's not intuitive.)