penumbra-zone / tower-abci

Tower-based ABCI interface.
MIT License
75 stars 24 forks source link

Support Unix domain sockets #35

Closed SuperFluffy closed 1 year ago

SuperFluffy commented 1 year ago

This patch allows the v037 server to serve cometbft connecting over unix domain sockets in addition to TCP.

Server::listen no longer has a tcp socket addr as argument. Instead, TCP and UDS listeners are set on ServerBuilder. They are complimentary and Server can serve requests over either or both protocols.

If neither TCP nor UDS are set ServerBuilder::finish returns None if neither are set. This seems in line with the current behavior of ServerBuilder, but more expressive errors would be nice (but not in this PR).

Note that when shuffling the code around I left the original commented out lines in place. Happy to remove them, but they probably serve a purpose.

Open question: should the v034 also support UDS?

erwanor commented 1 year ago

Thanks for the contribution! I will take a look right away. I assume that the motivation is doing IPC instead of a tcp socket over loopback to do away with the TCP overhead? I am curious about usecases in which this is a bottleneck.

At a glance, I think we want to only listen to either a tcp or uds stream and backport those changes to v034. I can do all of that today if you give me write access to your branch.

SuperFluffy commented 1 year ago

The TCP vs UDS bottleneck was not a concern (although it's interesting that Linux has roughly 10ms latency overhead compared to macOS on its loopback device).

I was writing integration tests with cometbft running as a child process and noticed that it supports unix:// in addition to tcp://.

This has a couple of advantages:

  1. You don't occupy yet another port when running unit tests; right now we have 2 for cometbft (rpc + p2p), and 2 for the app (its API service + the abci tcp socket). Extra mocked services would bring up further.
  2. Easier setup: just pass in a unix:///path/to/sock and use it throughout (no need to bind to :0 and check which port was assigned by the kernel.

And then of course it's nice when you consider socket activation if someone wants to run cometbft + tower-abci app as a raw process on top of systemd rather than k8s.

SuperFluffy commented 1 year ago

Adding support for unix streams is a good idea, but I think we can simplify the implementation. If you give me access to the branch, I can implement the requested changes myself and cut a release shortly after.

I gave you write access to astriaorg/tower-abci. Hope that will allow you to push to the feature branch.

erwanor commented 1 year ago

I have tested that uds support works for both v034 and v037. By the way, we are planning to add gRPC support sometime this fall!

erwanor commented 1 year ago

Released in v0.10.0, thanks for your contribution!

SuperFluffy commented 1 year ago

Thank you for merging this!