Unix Domain Socket transport #55

gabrik · 2020-11-20T10:14:24Z

This PR contains the code to implement the Unix Domain Sockets as transport for Zenoh

Creating a draft to keep track of the work

Implement Unix Domain Socket Link, LinkManager
Update locator with unix/<domain socket path>
Add unix feature for this transport
Add conditional building to build only on UNIX systems
Add tests

…domain socket path> Signed-off-by: gabrik <[email protected]>

Signed-off-by: gabrik <[email protected]>

…domain socket path> Signed-off-by: gabrik <[email protected]>

Signed-off-by: gabrik <[email protected]>

… fixed use in link/mod.rs Signed-off-by: gabrik <[email protected]>

JEnoch · 2020-11-20T13:14:53Z

I'm not sure "unix" is a good idea for the Cargo feature name as:

it's the same than the "unix" target name
on user's perspective, a "unix" feature doesn't sound clear on its purpose.
Why not "unix-socket" feature ?

Mallets · 2020-11-20T13:56:32Z

zenoh-protocol/tests/channel.rs

+    let _ = std::fs::remove_file("/tmp/socket.sock");
+    // Define the locator
+    let locators: Vec<Locator> = vec!["unix//tmp/socket.sock".parse().unwrap()];
+    // Define the reliability and congestgino control


Typo in congestion control.

Mallets · 2020-11-20T14:00:41Z

zenoh-protocol/tests/channel.rs

+fn channel_unix() {
+    let _ = std::fs::remove_file("/tmp/socket.sock");
+    // Define the locator
+    let locators: Vec<Locator> = vec!["unix//tmp/socket.sock".parse().unwrap()];


Change the file name to something more self-explanatory, e.g., zenoh-test-unix-socket.sock

Mallets · 2020-11-20T14:19:05Z

zenoh-protocol/src/link/locator.rs

@@ -63,19 +71,35 @@ pub enum Locator {
    Tcp(SocketAddr),
    #[cfg(feature = "udp")]
    Udp(SocketAddr),
+    #[cfg(all(feature = "unix", target_family = "unix"))]
+    Unix(PathBuf),


I suggest to call the locator UnixDomainSocket and not simply Unix, alternatively we can use Uds.

Mallets · 2020-11-20T14:20:04Z

zenoh-protocol/src/link/manager.rs

@@ -17,6 +17,8 @@ use async_std::sync::Arc;
 use crate::link::tcp::ManagerTcp;
 #[cfg(feature = "udp")]
 use crate::link::udp::ManagerUdp;
+#[cfg(all(feature = "unix", target_family = "unix"))]
+use crate::link::unix::ManagerUnix;


I suggest to call the ManagerUnixSocket instead of ManagerUnix

Mallets · 2020-11-20T14:21:35Z

zenoh-protocol/tests/channel.rs

+    // Define the locator
+    let locators: Vec<Locator> = vec![
+        "tcp/127.0.0.1:7449".parse().unwrap(),
+        "unix//tmp/socket_1.sock".parse().unwrap(),


Change the file name to something more self-explanatory, e.g., zenoh-test-unix-socket_1.sock

Mallets · 2020-11-20T14:21:49Z

zenoh-protocol/tests/channel.rs

+#[cfg(all(feature = "unix", target_family = "unix"))]
+#[test]
+fn channel_tcp_unix() {
+    let _ = std::fs::remove_file("/tmp/socket_1.sock");


Although /tmp is available in the totality of the system, I believe that using the current directory for the unix socket path is a better approach:

It is guaranteed that the cargo test will have write permissions while executing the test
System files are not created outside the test directory
I also suggest to use a more explanatory name for the socket, e.g., zenoh-test-unix-socket_1.sock

Another solution is use the tempfile crate that relies on std::env::temp_dir and performs automatic deletion of file.

I like the idea of using the tempfile crate. This should be added to the dev dependencies.

Mallets · 2020-11-20T14:22:01Z

zenoh-protocol/tests/channel.rs

+#[cfg(all(feature = "unix", target_family = "unix"))]
+#[test]
+fn channel_unix() {
+    let _ = std::fs::remove_file("/tmp/socket.sock");


Although /tmp is available in the totality of the system, I believe that using the current directory for the unix socket path is a better approach:

It is guaranteed that the cargo test will have write permissions while executing the test
System files are not created outside the test directory
I also suggest to use a more explanatory name for the socket, e.g., zenoh-test-unix-socket.sock

Mallets · 2020-11-20T14:23:14Z

zenoh-protocol/tests/channel.rs

+#[cfg(all(feature = "unix", target_family = "unix"))]
+#[test]
+fn channel_udp_unix() {
+    let _ = std::fs::remove_file("/tmp/socket_2.sock");


Change the file name to something more self-explanatory, e.g., zenoh-test-unix-socket_2.sock

Mallets · 2020-11-20T14:23:23Z

zenoh-protocol/tests/channel.rs

+    // Define the locator
+    let locators: Vec<Locator> = vec![
+        "udp/127.0.0.1:7449".parse().unwrap(),
+        "unix//tmp/socket_2.sock".parse().unwrap(),


Change the file name to something more self-explanatory, e.g., zenoh-test-unix-socket_2.sock

Mallets · 2020-11-20T14:26:11Z

zenoh-protocol/src/link/locator.rs

@@ -36,13 +38,17 @@ pub const PORT_SEPARATOR: char = ':';
 pub const STR_TCP: &str = "tcp";
 #[cfg(feature = "udp")]
 pub const STR_UDP: &str = "udp";
+#[cfg(all(feature = "unix", target_family = "unix"))]
+pub const STR_UNIX: &str = "unix";


My proposal for the locator protocol is to use "uds" from "unix domain socket". Other proposals are also accepted.

Mallets · 2020-11-20T14:46:24Z

zenoh-protocol/src/link/unix.rs

+/*************************************/
+/*              LINK                 */
+/*************************************/
+pub struct Unix {


We should avoid ambiguity on the usage of term Unix here. I suggest either to use:

UnixDomainSocket

Uds

Mallets · 2020-11-20T15:38:01Z

I'm not sure "unix" is a good idea for the Cargo feature name as:

it's the same than the "unix" target name

on user's perspective, a "unix" feature doesn't sound clear on its purpose.
Why not "unix-socket" feature ?

I would be tempted to go even further and have a clearer approach in the whole zenoh. Any feature related to a transport technology should be trasport_*. I.e., existing tcp and udp features should be also renamed to transport_tcp and transport_udp. What do you think?

Signed-off-by: gabrik <[email protected]>

implemented Unix Domain Socket transport, updated locator with unix/<…

624be33

…domain socket path> Signed-off-by: gabrik <[email protected]>

gabrik changed the title ~~Unix Domain Socket transpor~~ Unix Domain Socket transport Nov 20, 2020

gabrik added 8 commits November 20, 2020 11:34

added conditional build for unix domain sockets only in unix targets

fbf6b61

Signed-off-by: gabrik <[email protected]>

implemented Unix Domain Socket transport, updated locator with unix/<…

2803527

…domain socket path> Signed-off-by: gabrik <[email protected]>

added conditional build for unix domain sockets only in unix targets

98beaed

Signed-off-by: gabrik <[email protected]>

rebasing upon master, adding get_locators to ManagerUnix

cb77fae

Signed-off-by: gabrik <[email protected]>

rebasing upon master, adding get_locators to ManagerUnix

5bc60a3

Signed-off-by: gabrik <[email protected]>

run cargo fmt...

6418374

Signed-off-by: gabrik <[email protected]>

solving clippy issues, formatting

80b6df9

Signed-off-by: gabrik <[email protected]>

added tests, updated unix.rs with file removal upon listener removal,…

fd1057b

… fixed use in link/mod.rs Signed-off-by: gabrik <[email protected]>

gabrik marked this pull request as ready for review November 20, 2020 12:47

Mallets reviewed Nov 20, 2020

View reviewed changes

Mallets requested changes Nov 20, 2020

View reviewed changes

addressing comments

6e8d9cd

Signed-off-by: gabrik <[email protected]>

gabrik requested a review from Mallets November 23, 2020 09:52

fix typo, adding file removal step at end of unix domain socket tests

f8acca3

Signed-off-by: gabrik <[email protected]>

Mallets approved these changes Nov 23, 2020

View reviewed changes

Mallets merged commit beda767 into eclipse-zenoh:master Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unix Domain Socket transport #55

Unix Domain Socket transport #55

gabrik commented Nov 20, 2020 •

edited

Loading

JEnoch commented Nov 20, 2020

Mallets Nov 20, 2020

Mallets Nov 20, 2020 •

edited

Loading

Mallets Nov 20, 2020

Mallets Nov 20, 2020

Mallets Nov 20, 2020

Mallets Nov 20, 2020

JEnoch Nov 20, 2020 •

edited

Loading

Mallets Nov 20, 2020

Mallets Nov 20, 2020

Mallets Nov 20, 2020

Mallets Nov 20, 2020

Mallets Nov 20, 2020

Mallets Nov 20, 2020

Mallets commented Nov 20, 2020

Unix Domain Socket transport #55

Unix Domain Socket transport #55

Conversation

gabrik commented Nov 20, 2020 • edited Loading

JEnoch commented Nov 20, 2020

Choose a reason for hiding this comment

Mallets Nov 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JEnoch Nov 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mallets commented Nov 20, 2020

gabrik commented Nov 20, 2020 •

edited

Loading

Mallets Nov 20, 2020 •

edited

Loading

JEnoch Nov 20, 2020 •

edited

Loading