Hi Jacek,
On 29/05/2020 21:46, Jacek Caban wrote:
On 29.05.2020 18:52, Jacek Caban wrote:
Hi Gabriel,
On 26.05.2020 14:41, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescugabrielopcode@gmail.com
We have to treat a ref count of 0 in a special manner when they are linked to the script control, because the modules must exist even when they have no external refs (as the script control can still use them).
However, as evidenced by the tests, they must*not* keep a ref to the script control in this case, as it would create a circular ref and make no sense, anyway.
The alternative solution to allocate separate data for modules would require copying the data to multiple places (one for Module Collection, and one for orphaned Modules) which is more complicated, in my opinion.
There must be a cleaner way. If you need too much special treatment for orphaned modules, it means that the design needs to be improved. One thing that is often useful for similar cases are weak reference: never reference control from module and make sure that control always invalidates the module->control pointer when it actually releases the object.
Ok so, I'll give some examples to illustrate why it has to be done either this way (with refcount checks), or by allocating separate objects for modules when requesting an external ref (on top of the objects stored in the collection).
I will send a patch series with the latter (separate objects) to show the alternative solution, but it does complicate the code a bit and the enumerator as I mentioned initially... that's what I meant the first time and why I went with the refcount checks.
So, the tests show that the behavior is like this:
Module Collection is tied to Control. Obtaining a ref to the collection, then updating the control via put_Language, and using the older collection ref will refer to the new control (i.e. if it had 5 modules, then put_Language resets it back to 1, the old collection will also report 1 module and refer to it). So keeping it on the control is the easiest as it avoids pointless allocations in the code.
Modules are, obviously, tied to the collection (in an array for random access by index). A collection needs module names, for example. The control also needs access to the first (global) module to simplify the code. So modules always exist after put_Language, even if there's no explicit ref to them. In this case, they can't ref anything since the caller might only hold a ref to the control.
However, consider what happens if the caller/user obtains a ref to a module. First of all, this keeps the control alive, so releasing the ref to the script control in this case will not destroy it, you can still use the module ref to do stuff with it. So, a module obtained externally needs to hold a ref to the control.
Secondly, put_Language detaches the modules from the control (or collection). They lose their ref to the control, and operations on them return E_FAIL.
What I did with this patch series is to hold a refcount of 0 when no external (caller/user) refs are held to the module, but only the control/collection can access them (and their names). Increasing refcount to above 0 will obtain a ref to the control, since it means they have an external ref.
put_Language would detach them from the module, releasing the control ref only if the ref was above 0 (i.e. they had external ref), otherwise it would simply destroy the module, as it means they had no external ref.
The alternative I will send later works like this: keep an array of modules (without the interface) that keeps the module data in the control/collection. When an external ref is requested, we allocate (if it's not already, otherwise we share it) a module interface and have it keep a ref to the control and point to the module data in the array. put_Language would detach all the allocated objects from the control.
The end result is the same, but personally, I believe it introduces more complexity to the code, as we have more allocations to deal with, and more error handling (even in the enumerator). :-/
That's why I preferred the refcount check implementation, but anyway, I'll do the alternative so you can see how it looks like and if you prefer it.
Or maybe script module should just store ScriptHost reference.
It wouldn't change much and we need a ref to the control anyway (to start the script for example). Note that modules that haven't been detached by put_Language must keep the control alive if they have external refs.
Thanks, Gabriel