-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix response listener registration #90
Conversation
@sashacmc - I'm a little concerned about this change. There shouldn't be a reason to swap source and sink if I'm reading the zenoh uri mapping spec correctly. |
Yes, but from the client side of view server side sink and source will be reverted. |
Tagging @evshary for his feedback |
It would be really helpful to know more / have some examples, but I think I get what you are saying. Over Zenoh, the response to a request comes via the same "topic" string that was used for the request. This is different than what uProtocol expects (i.e that source and sink get swapped from request to response), and so the values have to be un-swapped when registering a callback listener for RPC responses. This suggests that the source and sink addresses in the response message are ignored when looking up the callback and, in essence, only the request ID is used. I have a few concerns about this. For one, we plan on replacing the one-callback-per-request model with a single callback for all requests. This would require non-trivial rework in this transport implementation even though both configurations should have near identical behavior per uProtocol spec. Additionally, this direct 1:1 mapping means that many other use cases in uProtocol may not work correctly; for example, it would not be possible to build an audit logger that listens to RPC requests and responses flowing in and out of a particular entity. The uProtocol spec allows for such a thing to be constructed - so long as it is authorized to do so, that logger would simply listen with the appropriate URI filters to receive copies of the messages. That does not appear possible with this model, particularly if the uProtocol spec is not followed with respect to the structure of the response message URIs. @stevenhartley - do you have any input on this? Have I misunderstood the spec? Is it expected that transports may optimize certain communication modes at the cost of message-bus-like capabilities (e,g, monitoring other topics)? |
Perhaps I didn't fully understand the difference between the one-callback-per-request model and a single callback for all requests. I can elaborate on how Zenoh implementation (at least Rust) works now, and then see whether it matches your expectations. The flow of RPC client:
RPC requests and responses are not available to record currently. We're using the mechanisms |
@evshary and @sashacmc - if the swapped positions of the source / sink filters is only used in Additionally, there may be multiple pending listeners that match a specific pattern if a particular RPC method receives a high volume of traffic, so that mechanism of guessing the match may also produce race conditions (unless the transport implementation is grabbing all matching callbacks). You also mention this as a step for
That seems to be overly complicated for detecting message type when the attributes structure contains a message type field. Shouldn't that be used instead within |
Both are fine for me. It's just a key for HashMap and is only used internally, so I don't have a strong opinion on where to do the swap.
Now I'm using Mutex to protect the callback hashmap to avoid that. Perhaps we need some benchmarks in the future to ensure this doesn't affect the performance too much.
You're right. This is a typo. I'm using the attributes structure to check message type in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely have some questions about how zenoh is being used to transport uProtocol messages, but I think they reach outside the scope of this PR. I'll write up my thoughts / questions to share in the appropriate forum.
For now, this change looks good to me. I'm going to verify that this gets the RPC test passing then I'll move forward with merging it.
This still does not appear to pass the RPC client/server tests. I tried swapping out the zenoh implementation to exchange messages only over the pub/sub path and it worked fine, so I think the messages, attributes, and URIs are all correct. I recommend rebasing to main to get the latest versions of the tests. |
7ada287
to
dcfc016
Compare
Hi @gregmedd, fixed, now test passes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Discussion on use of queries and layer 2 behaviors in layer 1 here: eclipse-uprotocol/up-spec#229
No description provided.