mbr / tinyrpc

A compact, modular transport and protocol agnostic RPC library. Does jsonrpc v2.
https://tinyrpc.readthedocs.org
MIT License
156 stars 53 forks source link

@lnoor, I now see a problem. `JSONRPCProtocol` class doesn't verify response IDs: #70

Closed lnoor closed 3 years ago

lnoor commented 5 years ago

@lnoor, I now see a problem. JSONRPCProtocol class doesn't verify response IDs: https://github.com/mbr/tinyrpc/blob/fb7490ef098d3ec42cb756ae97887300ddbccc65/tinyrpc/protocols/jsonrpc.py#L572

Theoretically it should reject responses with unexpected IDs.

Originally posted by @AndreiPashkin in https://github.com/mbr/tinyrpc/issues/69#issuecomment-472361973

lnoor commented 5 years ago

I suppose so. Actually the standard does not define how to handle an unsolicited response. What do you propose to do? Ignore the response or produce an error?

If you ignore it you won't know something went wrong and the client should keep waiting until it times out. But an error is questionable because the client did nothing wrong and the correct response may be still pending.

Perhaps the nicest solution is to introduce logging. Then this condition would generate a Warning message.

(I turned it in a separate ticket to keep discussion separate from issue #69)

AndreiPashkin commented 5 years ago

@lnoor, I think .parse_reply() must raise InvalidReplyError because such response does not conform to the standard and so that the client would have a chance to handle this situation somehow.

azebell commented 3 years ago

I agree with @AndreiPashkin. If an exception/error is not raised, the client will have no way to know that it needs to continue listening for the response of the request it had sent.

Currently, if an unsolicited response is received, the client will accept it and treat it as if it were the response that corresponds to the client's most recent request, which isn't necessarily true. There is no mechanism in place to handle the case of an unsolicited response, so an update to "continue awaiting response" must be implemented.