Use reqid and not ID #40
Conversation
response reqid is the same as the request.id so we need to use this in lieu of id.
There was a problem hiding this comment.
This change looks good to me.
I have used the RPC client / server in the example repo to check that this does not introduce any new issues that were not already present. Full confirmation will require changes from #34.
For the sake of record keeping, tests are missing because the relevant test (testRpcClient) is known to be broken right now. We are actively working to address that.
There was a problem hiding this comment.
I have just gone back and retested everything, and I'm certain that, either this check is not needed, or it would need to made with an associated change in QueryHandler. I've put inline comments at the two code sites in my PR. To reiterate that here, the purpose of the queryMap_ is to tranport the zenoh z_query_t from the incoming zenoh call to the place in sendQueryable() (called from onReceive()) where its needed for reply. The zenoh call-reply is not based on uProtocol attributes. The only role of the id() or reqid() is its uniqueness as a map key.
In the long run, this queryMap_ should be removed. A better mechanism should be provided to pass the z_query_t around the onReceive() opaqueness. One such mechanism is thread local storage. There is no reason to slow the rpc code down with unnecessary associative array lookups.
Also, when reqid() was used, it appeared to be all zeros coming from the client side, and that would not be a useful unique key. I think that is probably expected, but I will confirm that again.
|
No longer needed, got pulled in from #34 that was merged |
response reqid is the same as the request.id so we need to use this in lieu of id.
Open source gods please forgive me for no test....