search

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

Request ID generator may produce memory leaks #69

Closed AndreiPashkin closed 3 years ago

AndreiPashkin commented 5 years ago

There is a private method JSONRPCProtocol._get_unique_id() that is responsible for generating unique IDs for RPC client requests: https://github.com/mbr/tinyrpc/blob/2b2857d1947e7019bb01e50badc801194684226b/tinyrpc/protocols/jsonrpc.py#L476

It generates ID's by addition which may lead to a memory leak in case if client stays alive for a long time (month? year?). I believe some kind of sliding-window algorithm should be used there instead.

lnoor commented 5 years ago

Using a generator is a great suggestion. The default could replace the current implementation just incrementing a counter. The default generator can then be replaced by your own generating uuids for example.

AndreiPashkin commented 5 years ago

I could make a PR with JSONRPCProtocol class updated with option to provide custom ID generator.

lnoor commented 5 years ago

Please do! But consider this:

  1. default generator should generate same sequence as the current mechanism
  2. alternate generator should be passed in constructor
  3. alternate generator should not be replaceable (no set_generator() method)
  4. also provide test(s) and documentation

1-2: to prevent breaking changes 3: to discourage swapping generators mid-flight (only for those who know what they're doing)

AndreiPashkin 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.