Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

Add support for unix sockets #47

Closed
wants to merge 3 commits into from
Closed

Conversation

wews64
Copy link

@wews64 wews64 commented Feb 8, 2019

resolve #30

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@melekes
Copy link
Contributor

melekes commented Feb 15, 2019

/cc @davebryson @tomtau

@davebryson
Copy link
Contributor

@wews64 @melekes Thanks! looks good!
I have one suggestion. Why not just parse the address and simply select the appropriate listener versus duplicating code here:
https://github.com/tendermint/rust-abci/pull/47/files#diff-4ce93534efc34e923ce01e975eb7ed80R14

Example: if the address starts with unix use the UnixListener, otherwise use the tcp listener. The rest of the code stays the same and you can eliminate the whole serve_unix function

@wews64
Copy link
Author

wews64 commented Feb 17, 2019

@davebryson
I can not remove duplicating code, because there different types of incoming connections std::os::unix::net::UnixStream and std::net::TcpStream. I can make common interface for both functions, something like pub fn serve<A>(app: A, addr: &str) -> io::Result<()>. But it, under the hood, will also call different handlers.

@tomtau
Copy link
Contributor

tomtau commented Feb 18, 2019

@wews64 I'm not sure whether this works, but you could possibly pass in the listener as an argument, constructed based on the address type. Both UnixStream and TcpStream implement IntoIterator, so perhaps the listener argument could have a type like IntoIterator<Item=Result<S, E>> (where S is the stream, so something like S: Read + Write) and instead of:

for new_connection in listener.incoming() {
...

you could do:

for new_connection in listener.into_iter() {
...

@wews64
Copy link
Author

wews64 commented Feb 18, 2019

@tomtau but on next step i'm must send specific type (UnixStream or TcpStream) in AbciStream::from.

@tomtau
Copy link
Contributor

tomtau commented Feb 19, 2019

@wews64 just a correction -- when I wrote UnixStream and TcpStream, I meant UnixListener and TcpListener. For streams themselves, what if you constrain S more? Something like S: Read + Write + Into<AbciStream>?

@tomtau
Copy link
Contributor

tomtau commented May 29, 2019

@wews64 there has been quite a lot of changes in the meantime (e.g. tokio was added), so will close this PR for the moment -- feel free to update your changes and open a new PR

@tomtau tomtau closed this May 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for unix sockets
4 participants