Prototype listening on SocketAddr instead of port (WIP) #4

dusty-phillips · 2020-06-05T11:29:22Z

The basic idea is the same, but it better mimics
the normal socket interface.

The Switchboard can now be thought of as
a parody of the entire internet
instead of a single machine.

Most everything is a direct mapping,
but there are two uncertain situations:

When connect() is called, there is no clear indicator
as to where the client is connecting from.
So if we ever wanted to implement e.g. peer_addr
on a MemorySocket,
the behaviour would be undefined.
It is not clear what address to listen on when
connecting to 0.0.0.0.

Both of these could be solved by mapping to the local
machine address (using e.g the get_if_addrs crate),
but I'm not sure that make sense.
Another option I toyed with was having a "set_connect_address"
global, but that simply lacked elegance.

( Discussion in #3 )

The basic idea is the same, but it better mimics the normal socket interface. The Switchboard can now be thought of as a parody of the entire internet instead of a single machine. Most everything is a direct mapping, but there are two uncertain situations: * When connect() is called, there is no clear indicator as to where the client is connecting *from*. So if we ever wanted to implement e.g. peer_addr on a MemorySocket, the behaviour would be undefined. * It is not clear what address to listen on when connecting to 0.0.0.0. Both of these *could* be solved by mapping to the local machine address (using e.g the get_if_addrs crate), but I'm not sure that make sense. Another option I toyed with was having a "set_connect_address" global, but that simply lacked elegance. ( Discussion in bmwill#3 )

bmwill

Sorry this took me a little bit to get to. I think some of my concerns mostly stem from the awkwardness of how to handle the ip portion of a socketaddr and I'm remembering more and more why i punted on it at the time :)

I think this is on the right track though I'm not 100% sure what the right way to handle some of the edge API cases would be (ie ToScoketAddr doing hostname resolution), it just might take some more time to think through it.

Cargo.toml

src/lib.rs

bmwill

This is looking much closer. There's just a couple tweaks left and then this should be good!

bmwill · 2020-06-10T22:06:33Z

src/async.rs

@@ -41,7 +41,7 @@ impl MemoryListener {
        IncomingStream { inner: self }
    }

-    fn poll_accept(&mut self, context: &mut Context) -> Poll<Result<MemorySocket>> {
+    pub fn poll_accept(&mut self, context: &mut Context) -> Poll<Result<MemorySocket>> {


I don't think this needs to be made public?

It's public in tokio, so I'm not sure. accept is also async in tokio, but not in this impl, so I'm not sure how close to match it.

bmwill · 2020-06-10T22:09:46Z

src/lib.rs

+        // Similarly, it doesn't make a sense to listen on "all interfaces"
+        // in this environment, so return an error if they requested 0.0.0.0
+        // TODO: We could use get_if_addrs and use the host's real name?


This comment could probably be tweaked a bit, I'm not sure what "similarly" is referring to in this context.

Must have been referring to something that got deleted when I switch to SocketAddr. Will reword.

src/lib.rs

bmwill · 2020-06-10T22:16:42Z

src/lib.rs

+        if let Some(sender) = switchboard.0.get_mut(&address) {
+            let (socket_a, socket_b) = Self::new_pair();
+            // Send the socket to the listener
+            sender
+                .send(socket_a)
+                .map_err(|_| ErrorKind::AddrNotAvailable)?;
+
+            Ok(socket_b)
+        } else {
+            Err(ErrorKind::AddrNotAvailable.into())
+        }


This is probably a little bit easier to read if you remove the if let and just map the get_mut result to an error with ok_or_else and ? like was done before.

I believe the only thing that needs to be changed here is just changing from port to address, the construction of the new socket and sending one half over the channel shouldn't need to be changed.

bmwill · 2020-06-10T22:18:45Z

src/lib.rs

+    pub fn local_addr(&self) -> Result<SocketAddr> {
+        Ok(self.address)


I'm not sure if there is value in having this returning a result since its an infallible call. You might have had some reason to tweak it though

It returns a Result in std::net and tokio::net, so I was going for drop-in symmetry. I'm happy to go either way, though; your call.

dusty-phillips · 2020-06-11T11:31:01Z

Made most of the requested changes; awaiting guidance on whether to match the other implementations for the public poll_accept and local_addr Result.

bmwill · 2020-06-11T16:04:26Z

I think that since getting local_addr can't fail it makes most sense to keep it as being an infallible api. I would think that it would be fairly easy to Ok wrap it if you needed to implement a generic socket trait for it.

I also think that poll_accept should be kept as private. If you need to have an equivalent accept function it should be easy enough to introduce one as an async fn the only issue is that it can't really be called accept since there's already the sync accept:

    pub async fn accept_async(&mut self) -> Result<MemorySocket> {
        futures::future::poll_fn(|ctx| self.poll_accept(ctx)).await
    }

If this isn't needed now then it could probably wait to be added at a later point in time (mostly because it doesn't really relate to converting the lib to use SocketAddrs which is the primary focus of this PR)

dusty-phillips · 2020-06-17T09:42:37Z

Should have hit all the points hit now; I had them in my local directory for several days but apparently forgot to push. 🤦‍♂️

bmwill · 2020-06-28T16:53:42Z

Sorry this took me a bit to get back to, thanks for making those tweaks!

bmwill self-requested a review June 9, 2020 15:36

bmwill reviewed Jun 9, 2020

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

dusty-phillips force-pushed the socketaddr branch from d489cad to 2b14545 Compare June 10, 2020 20:15

Remove rand dependency and drop ToSocketAddrs

40ad6c4

dusty-phillips force-pushed the socketaddr branch from 2b14545 to 40ad6c4 Compare June 10, 2020 20:19

bmwill reviewed Jun 10, 2020

View reviewed changes

dusty-phillips force-pushed the socketaddr branch from 0fc22e2 to 6df9301 Compare June 12, 2020 12:01

nits

c3da08b

dusty-phillips force-pushed the socketaddr branch from 6df9301 to c3da08b Compare June 17, 2020 09:41

bmwill merged commit 74110b1 into bmwill:master Jun 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prototype listening on SocketAddr instead of port (WIP) #4

Prototype listening on SocketAddr instead of port (WIP) #4

dusty-phillips commented Jun 5, 2020

bmwill left a comment

bmwill left a comment

bmwill Jun 10, 2020

dusty-phillips Jun 11, 2020

bmwill Jun 10, 2020

dusty-phillips Jun 11, 2020

bmwill Jun 10, 2020

bmwill Jun 10, 2020

dusty-phillips Jun 11, 2020

dusty-phillips commented Jun 11, 2020

bmwill commented Jun 11, 2020

dusty-phillips commented Jun 17, 2020

bmwill commented Jun 28, 2020

		pub fn local_addr(&self) -> Result<SocketAddr> {
		Ok(self.address)

Prototype listening on SocketAddr instead of port (WIP) #4

Prototype listening on SocketAddr instead of port (WIP) #4

Conversation

dusty-phillips commented Jun 5, 2020

bmwill left a comment

Choose a reason for hiding this comment

bmwill left a comment

Choose a reason for hiding this comment

bmwill Jun 10, 2020

Choose a reason for hiding this comment

dusty-phillips Jun 11, 2020

Choose a reason for hiding this comment

bmwill Jun 10, 2020

Choose a reason for hiding this comment

dusty-phillips Jun 11, 2020

Choose a reason for hiding this comment

bmwill Jun 10, 2020

Choose a reason for hiding this comment

bmwill Jun 10, 2020

Choose a reason for hiding this comment

dusty-phillips Jun 11, 2020

Choose a reason for hiding this comment

dusty-phillips commented Jun 11, 2020

bmwill commented Jun 11, 2020

dusty-phillips commented Jun 17, 2020

bmwill commented Jun 28, 2020