On Mon Aug 21 10:23:13 2023 +0000, Jacek Caban wrote:
The wider implication of that is that we won't be able to unify `Release` implementation and we'd need to continue supporting custom per-object implementation to be able to put hacks there. Also, well, it's clearly a workaround for a design shortcoming. It does not help to gain confidence in the design if you need workarounds in the very first MR after introducing the new architecture. It's a perfect moment to get rid of workaround, not introduce new ones. We analyzed it and we know potential solutions, so it will be fine. But to asses the solution, I'd be interested in how remaining objects will look like and if there are more similar problems. Right now, to verify your claims, I'd need to review dozens of object. I will need to review them later again when you will be migrating them, so doing it now would be a waste of my time. Reordering patches on your side should be trivial.
I looked at the other objects and they don't seem to have this problem. I will reorder them regardless to get it moving, but with respect to this issue, I don't think it's that much of a hack.
It makes sense to discard the tasks associated with the object when its ref becomes 0, whether the object gets freed or not immediately after that, because they shouldn't access a 0 refcount object.
And the whole point of `task_magic` was to avoid having refs from the task back to the object, right? Otherwise the object would never even reach 0 refcount as long as a task exists.
As for a unified implementation of Release, we could always add some sort of vtbl entry (method or just offset in struct to task_magic?) to deal with it, but that's just one potential solution to this. Maybe we can also create cyclic ref between task and object and avoid task_magic at all. Not sure how you feel about that, though.