search

capnproto / pycapnp

Cap'n Proto serialization/RPC system - Python bindings
BSD 2-Clause "Simplified" License
463 stars 124 forks source link

Instanciate one bootstrap object per client, and add IP information #209

Open Cheaterman opened 4 years ago

Cheaterman commented 4 years ago

See #169 for reference

As already discussed in #208 :

[...] it would be really important to give some access to the client IP/port somewhere
- it seems like one of the most idiomatic ways to do this is to instance one bootstrap
object per client (I discussed this with Kenton back in the day, and IIRC he said that's
what people do in C++ - one object per client, not a single shared instance as it is
now in pycapnp).

Indeed, as you can see here: https://github.com/capnproto/pycapnp/blob/master/examples/calculator_server.py#L134 , we currently instantiate a single bootstrap object which will then be given to all clients. This would be OK (since we can then instantiate per-client caps as needed) if we didn't need any information about the IP/port the client is connecting from. However, this is very typically needed (for example when implementing IP-based whitelisting or blacklisting), and C++ clearly has access to this information, so I don't see why us Pythonistas shouldn't :-).

As previously discussed: from what I've seen, a good way to address this would be to instance one bootstrap object for each client that connects, and as mentioned in #169 (and previously referred to above from #208 ): This implies having a single bootstrap object per client, which breaks backwards compatibility (but as I understand it, the proper C++ way is also to instance a bootstrap object for each client).

So as I suggested in #169 , what we might want to do is use a on_connect method on the bootstrap object (for which the name could be customized using a __connect_handler__ attribute on the bootstrap class), as demonstrated here: https://github.com/Cheaterman/capnchat/blob/master/server.py#L173 this method being called from: https://github.com/capnproto/pycapnp/pull/169/files#diff-e71cd89847181d44235a7c4f42851d88 (see L76-79 - notice how the __connect_handler__ attribute can be used to change which method will be called)

Thanks in advance for your consideration :-)

haata commented 4 years ago

Yeah, I think this would be a good idea. It may take me a bit get started on this (focused on a few other projects right now). Though also happy to review PRs and give advice.

Cheaterman commented 4 years ago

FWIW, we could probably implement this without breaking backward compatibility by using the connect handler (default on_connect) directly on our current singleton bootstrap instance.

EDIT: I'll be happy to reopen my PR and update it, also provide one flavor that breaks backward compatibility and one that doesn't, this way making a choice will be easier :-)