eclipse-uprotocol / up-spec

uProtocol Specifications
Apache License 2.0
31 stars 25 forks source link

Cannot Correlate Response messages to the correct registere UListener #180

Closed stevenhartley closed 2 months ago

stevenhartley commented 2 months ago

Misha brought up an important issue when he was reviewing up-java implementation of 1.5.8 here: https://github.com/eclipse-uprotocol/up-java/pull/111#discussion_r1628409840 and this would apply to all languages and implementations.

RpcClient::invokeMethod() calls UTransport::registerListener() passing source (my address) and sink (method to invoke) such that the transport can match a response message for the right UListener. If the client calls invokeMethod() again for the same method, we would register another listener. When the response comes back to the transport we would have two listeners for the same pattern but no way to correlate which listener is the right one to call as reqid is not passed to the transport. This issue also is possible because UTransport specifications allows for it in the requirement:

To address this we either have to also pass id in the registerListener() (and/or other portions of UAttributes) or we only allow one call at a time and specify a common return code when someone tries to invoke the same message multiple times (i.e. registerListener() returns UCode.ALREADY_EXISTS).

comments/suggestions?

gregmedd commented 2 months ago

My plan in up-cpp was for there to be a single registerListener() for all RpcClient instances within a uE, then each individual response would be mapped to the correct request callback within that listener.

However, adding an optional request ID filter to registerListener() at the UTransport level would certainly remove the need for that. The cost is increased transport implementation complexity.

stevenhartley commented 2 months ago

My plan in up-cpp was for there to be a single registerListener() for all RpcClient instances within a uE, then each individual response would be mapped to the correct request callback within that listener.

I like that solution even better, need to see how I can fit that into the RpcClient that I've implemented on the Java side.

matthewd0123 commented 2 months ago

One idea:

We add a map to a UListener to track requests and responses. So, when the first invoke method is called, the listener is initialized, and inside, a map is created, with one entry containing the ID. Then, when a response is received with the reqid matching the id of the request, a specific on_receive can be triggered. If another invoke_method is invoked with the same listener, then we just add that new id to the map, with the same or different behavior. The benefit of this would be that we don't have to alter the signatures of register_listener, but it would add a fair amount of complexity to listeners.

Greg's idea is good though, and may be simpler. Having one manager for the responses would be nice...

stevenhartley commented 2 months ago

Chatting with Greg we found a simple solution that will work for Java so I'll close this issue as it is just something that we need to take care of in the language libraries individual RpcClient implementation.

For those who are interested, I'm not going to change uTransport and I don't want to add state to the L2 APIs (they will continue to be stateless static methods) so if a client calls uTransport 100 times the same method, we will have 100 listeners and inside of the listener we will do the ID correlation as this is something that needs to be done at L2. If this becomes a performance problem in Java, I'll explore other options. Thanks @gregmedd & @matthewd0123 for your feedback! Also will fix the calls to unregisterListener (was missing in my code) when the future is completed (for any reason).

tamarafischer commented 2 months ago

I know this is closed but I was away from keyboard for a day. I have been seeing a lot of complexity around listeners for quite a while and would like to suggest what I implemented. uTransport is registered with listeners. UTransport decides when the listener is called - when a message arrives The listener decides if the listener is called - the listener has an isMatch(UAttributes) method that returns a boolean and decides if this listener is a match to the message using the header information only. This makes it easy to test, keeps the complexity out of uTransport.