Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Unix domain sockets #35

Merged
merged 6 commits into from
Sep 7, 2023
Merged

Support Unix domain sockets #35

merged 6 commits into from
Sep 7, 2023

Conversation

SuperFluffy
Copy link
Contributor

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?

@SuperFluffy SuperFluffy changed the title Uds Support Unix domain sockets Sep 6, 2023
@erwanor
Copy link
Member

erwanor commented Sep 6, 2023

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
Copy link
Contributor Author

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.

Copy link
Member

@erwanor erwanor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/v037/server.rs Outdated Show resolved Hide resolved
src/v037/server.rs Outdated Show resolved Hide resolved
src/v037/server.rs Outdated Show resolved Hide resolved
@SuperFluffy
Copy link
Contributor Author

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
Copy link
Member

erwanor commented Sep 7, 2023

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 erwanor merged commit e897a33 into penumbra-zone:main Sep 7, 2023
4 checks passed
@erwanor
Copy link
Member

erwanor commented Sep 7, 2023

Released in v0.10.0, thanks for your contribution!

@SuperFluffy
Copy link
Contributor Author

Thank you for merging this!

conorsch added a commit that referenced this pull request Oct 13, 2023
Follow up to the addition of unix socket support [0], making that
feature conditional on building for a target OS family of "unix" [1].

[0] #35
[1] https://doc.rust-lang.org/reference/conditional-compilation.html#target_family
erwanor pushed a commit that referenced this pull request Oct 13, 2023
Follow up to the addition of unix socket support [0], making that
feature conditional on building for a target OS family of "unix" [1].

[0] #35
[1] https://doc.rust-lang.org/reference/conditional-compilation.html#target_family

Co-authored-by: Conor Schaefer <conor@penumbralabs.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Testnet 56: Callisto
Development

Successfully merging this pull request may close these issues.

2 participants