Fix shutdown ordering for OOP IPlugin Release - #2171
Conversation
|
… into oop-shutdown-iplugin-order
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
docs/plugin/execution-modes/outofprocess.md:12
- Docs: this section uses "out of process" while the rest of the document uses the hyphenated "out-of-process". Using consistent terminology improves readability and avoids confusion with the established term used elsewhere in this page.
It is also possible to run the full plugin out of process. This allows a plugin to be hosted by `ThunderPlugin` even if it was not split or specifically designed for the traditional OOP plugin model. In this mode, `PluginHost::IPlugin` itself is hosted by `ThunderPlugin`.
There was a problem hiding this comment.
@nxtum I get the idea, and I think it looks good in general, but I am just wondering about one corner case:
Since _parent. Stop() is no longer called in Dispatch() for a fully OOP plugins, what if the connection was no longer valid for some reason, and then we would not get to _connection->Terminate() in ReleaseInterfaces()?
Well, there are 2 diff cleanup paths
When Thunder first creates this OOP plugin, it stores the remote connection https://github.com/rdkcentral/Thunder/blob/master/Source/Thunder/PluginServer.h#L1343-L1354 https://github.com/rdkcentral/Thunder/blob/master/Source/com/Communicator.h#L1359 This returns a ref counted connection obj, which is owned until its released by ReleaseInterfaces So from my understanding, during the deactive part, when you terminate this _connection at the end, it will still exist as its still a stored connection even if the channel was closed |
|
For a "traditional" OOP plugin, Stop() is still called. Only when you run a plugin that was not designed for OOP you cannot Stop() because the remote object is Iplugin itself, thunder could still be releasing IPlugin proxy. So if you were to stop there, there is a race, which is why you get: [Error]: Could not remote release the Proxy for Interface [0x30] |
VeithMetro
left a comment
VeithMetro
left a comment
There was a problem hiding this comment.
Let's add an ASSERT like we discussed, and please do a smoke test to see it does not fire 😄
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
docs/plugin/execution-modes/outofprocess.md:14
- Use consistent hyphenation for execution modes: this section uses “out of process” / “in process”, while the rest of the docs use “out-of-process” / “in-process”. Keeping the terminology consistent improves searchability and avoids ambiguity.
It is also possible to run the full plugin out of process. This allows a plugin to be hosted by `ThunderPlugin` even if it was not split or specifically designed for the traditional OOP plugin model. In this mode, `PluginHost::IPlugin` itself is hosted by `ThunderPlugin`.
This is mainly useful as a development and debugging option. For example, it can be used to isolate leaks, crashes, or shutdown behavior in plugins that were originally designed to run in process.
|
added an ASSERT that would fire if:
|
|
And just to make sure, you did smoke test with this ASSERT and it does not fire? 😄 |
We will find out in production 😄 I tested spamming activate/deactivate on multiple in process and oop plugins and didn't fire, so should be good to go |
Fix shutdown of plugins where IPlugin itself is hosted by ThunderPlugin OOP
The failing release was the host releasing the plugin interface itself
ReleaseInterfaces() could terminate _connection before releasing currentIF
ThunderPlugin could call WorkerPool:Stop() from the ServiceAdministrator callback when the final IPlugin object was destroyed
That allowed the ThunderPlugin process to close while the host was still waiting for the remote currentIF->Release() to complete...