Send notification to Skeleton on Proxy destruction to "Unsubscribe" method.#358
Conversation
Tejveerpratap2803
requested review from
LittleHuba,
bemerybmw,
castler,
crimson11,
hoe-jo and
limdor
as code owners
Tejveerpratap2803
force-pushed
the
tepr_proxy_method_unsubscribe_notification
branch
2 times, most recently
from
5efbd22 to
11e9903
Compare
Tejveerpratap2803
changed the title
Tejveerpratap2803
force-pushed
the
tepr_proxy_method_unsubscribe_notification
branch
3 times, most recently
from
aae5669 to
53393c8
Compare
Tejveerpratap2803
force-pushed
the
tepr_proxy_method_unsubscribe_notification
branch
5 times, most recently
from
30fdce5 to
3abf06f
Compare
Tejveerpratap2803
force-pushed
the
tepr_proxy_method_unsubscribe_notification
branch
4 times, most recently
from
6ca8e82 to
a9c92e1
Compare
Tejveerpratap2803
force-pushed
the
tepr_proxy_method_unsubscribe_notification
branch
4 times, most recently
from
caf69c5 to
680061a
Compare
Tejveerpratap2803
force-pushed
the
tepr_proxy_method_unsubscribe_notification
branch
5 times, most recently
from
84ee94a to
fa47c66
Compare
…ethod - Add OnProxyMethodUnsubscribeFinished to notify SkeletonMethod when Proxy is destroyed - Add MethodUnsubscriptionRegistrationGuard for cleanup management - Add MethodResourceMap for tracking method resource ownership per proxy instance - Refactor: use ProxyMethodInstanceIdentifier as key in registration_guards_ (resolves intermediate solution from issue-250236)
Tejveerpratap2803
force-pushed
the
tepr_proxy_method_unsubscribe_notification
branch
from
fa47c66 to
63f72b5
Compare
There was a problem hiding this comment.
For the future reference please split your PRs in thematically meaningful commits to make the review easier.
In the description you outline 6 major bullet points that have been addressed in this PR. Some of this points are sufficiently different that they could have gotten their own commits.
Tejveerpratap2803
force-pushed
the
tepr_proxy_method_unsubscribe_notification
branch
from
1fe141b to
383564c
Compare
Tejveerpratap2803
force-pushed
the
tepr_proxy_method_unsubscribe_notification
branch
from
383564c to
6c96a95
Compare
| const auto method_shm_path_name = GetMethodChannelShmName(); | ||
| memory::shared::SharedMemoryFactory::Remove(method_shm_path_name); | ||
| method_shm_resource_.reset(); | ||
| memory::shared::SharedMemoryFactory::RemoveStaleArtefacts(method_shm_path_name); |
There was a problem hiding this comment.
I don't think this should be called here. It's basically doing the same thing as Remove above except it's intended to be called when the memory region was not created by the process calling it (which is not the case here).
|
|
||
| // Subscription state is tracked directly in Proxy (not via ProxyMethod references) because ProxyMethod | ||
| // objects may have already been destroyed by the time TeardownMethods() is called from ~Proxy(). | ||
| const bool is_subscribed = are_proxy_methods_subscribed_.load(); |
There was a problem hiding this comment.
We currently have a race condition between are_proxy_methods_subscribed_ being set and checking it here. It's possible that after a skeleton re-offers, we will call SubscribeServiceMethod and before we set are_proxy_methods_subscribed_ to true, the destructor is called and we check the value here which would still be false.
I think we want to make sure that this function is never called concurrently with the ServiceAvailabilityChangeHandler to avoid any race conditions. So before TeardownMethods is called, we should call find_service_guard_.reset() or better yet, create a guard object which calls the methods cleanup on destruction and ensure that it is destroyed after the find_service_guard_.
| } | ||
| method_subscription_registration_guard_qm_.emplace(std::move(qm_registration_result).value()); | ||
|
|
||
| // Register an unsubscription handler for QM proxies. The handler uses the same scope as the subscribe handler |
There was a problem hiding this comment.
The PrepareOffer function is getting extremely long. Can you please create a new function called RegisterMethodHandlers or something similar? I think it should take a quality type and return a pair of subscribe / unsubscribe handler registration guards. You can then call it for qm and optionally for asil b and store the results in member variables if there are no errors in either (that way you don't have to manually reset the qm guards in case the asil b registration fails).
3 participants
Notify Skeleton on Proxy destruction to unsubscribe method resources
When a Proxy is destroyed, TeardownMethods() now sends an UnsubscribeServiceMethod IPC message to the Skeleton, which removes the proxy's shared memory region and unregisters all method call handlers for that proxy.