Conversation
crimson11
requested review from
LittleHuba,
bemerybmw,
castler,
hoe-jo and
limdor
as code owners
crimson11
force-pushed
the
mf_subscription_state_change_handler
branch
6 times, most recently
from
d25466a to
a376f74
Compare
| "Invalid subscription state machine state."); | ||
| return SubscriptionState::kSubscribed; | ||
| } | ||
| return SubscriptionStateMachineState2SubscriptionState(current_state); |
There was a problem hiding this comment.
i like this name but i think we do not follow this convention of using a digit in the Method names,
best to use SubscriptionStateMachineStateToSubscriptionState
| auto subscription_state_change_handler = [](SubscriptionState new_subscription_state) { | ||
| return true; | ||
| }; | ||
| state_machine_.SetSubscriptionStateChangeHandler(subscription_state_change_handler); |
There was a problem hiding this comment.
i dont see any explicit expectations or conditions in this test,
does having this really add value?
There was a problem hiding this comment.
Added comment, which clarifies the need/idea.
| * \brief Unsets/Unregisters a SubscriptionStateChangeHandler for this event. After this method returns, it is | ||
| * guaranteed, that the previously registered handler is neither active nor will be called anymore. | ||
| */ | ||
| Result<void> UnSetSubscriptionStateChangeHandler() noexcept; |
There was a problem hiding this comment.
minor nitpick ;)
we have UnSet upper case S in the public API and other places have lowercase S,
|
|
||
| /* broken_link_c/issue/21466236 */ | ||
| ScoreReq.CompReq BehaviourOfUnsetSubscriptionStateChangeHandler { | ||
| description = '''{{UnsetSubscriptionStateChangeHandler}} {{UnsetSubscriptionStateChangeHandler}}{{SubscriptionStateChangeHandler}}will unregister the {{SubscriptionStateChangeHandler}} (See SubscriptionStateChangeHandler) that was registered by the class instance with a call to {{SetSubscriptionStateChangeHandler}} After calling the will no longer be triggered when the {{SubscriptionState}} (See progress-cursor SubscriptionState changes. |
There was a problem hiding this comment.
not changed in this PR,
but is here
{{UnsetSubscriptionStateChangeHandler}} {{UnsetSubscriptionStateChangeHandler}}{{SubscriptionStateChangeHandler}}
is expected to be like this ?
{{UnsetSubscriptionStateChangeHandler}} {{SubscriptionStateChangeHandler}}
?
There was a problem hiding this comment.
This is broken anyhow :) because this is copied over from AUTOSAR requirements - I do not even get all these curly braces! But I cleaned up the mess a bit ;)
|
|
||
| /** | ||
| * \api | ||
| * \brief Sets/Registers a SubscriptionStateChangeHandler for this event. This handler will be called whenever the |
There was a problem hiding this comment.
Just a nitpick,
should this be called as field ?
or since it dispatches to the event_base it is still accurate to call this a event?
(from other methods we use field instead of event even if we dispatch to the event_base.
| /// might also require the same lock, which is not guaranteed to be recursive, thus leading to a deadlock. If the user | ||
| /// intends to do such activities from this callback, he shall dispatch it to a separate thread. | ||
| /// However, unsetting the handler within its call is a reasonable/supported use-case. Instead of calling | ||
| /// UnsetSubscriptionStateChangeHandler, the user shall return false from the handler. See return-value description. |
There was a problem hiding this comment.
eventhough we warn users about the consequence of deadlock,
is it better or worth doing something like we do with "is_in_receive_handler_context" if we are in the same ctx then we dont acquire the lock to prevent deadlock?
There was a problem hiding this comment.
Checking/verifying it comes at additional costs! We need to manage a thread-local variable to monitor the mis-use.
Here we are basically adding an AoU - like many other AoUs we have and where we expect the user to adhere to. I.e.: We disallow, that a user calls APIs of a mw::com proxy/skeleton/event/field instance concurrently. We do NOT supervise this ... although we could do this! So - it is the same here! We don't supervise, that the user follows our AoU.
SetReceiveHandler is a "different animal". Because there we explicitly allow, that the user calls UnsetReceiveHandler from within the handler-context! Thus, we have to detect it, because we then have different code paths for UnsetReceiveHandler depending on the context. And the only reason is: In case of ReceiveHandler I wasn't smart enough to come up with the same idea as in SubscriptionStateHandler -> if you want "unset" it, just tell it via return-value. The underlying code is so much more effective! I'm tempted to change the signature/idea for ReceiveHandler maybe the same at some time ...
Implementation of ProxyEvent/Field SetSubscriptionStateChangeHandler and the corresponding Unset-API. Introduce SubscriptionStateChangeHandler with revised signature, which supports implicit Unset via return value.
Updated requirements for SubscriptionStateChangeHandler
Updated design to clarify, how SubscriptionStateChange handlers work.
The API to set/unset a SubscriptionStateChangeHandler is now also added to the Field.
Incorporated review comments: Fixed naming to always use UnsetStateChangeHandler(). Extended unit tests.
crimson11
force-pushed
the
mf_subscription_state_change_handler
branch
from
63b6c36 to
3fe9989
Compare
2 participants
Although we had already an explicit requirement for the support of ProxyEvent/Field side
SetSubscriptionStateChangeHandler()and the related Unset-API implementation was stillmissing.
This PR provides the implementation and updates the requirements and design accordingly.