Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

Reuse UDP port when possible #34

Closed
wants to merge 4 commits into from
Closed

Conversation

cannium
Copy link
Contributor

@cannium cannium commented Oct 17, 2018

Try to mimic the behavior of libp2p/go-reuseport-transport

Fix #8

@cannium cannium force-pushed the port-reuse branch 3 times, most recently from 432029a to 89f0a90 Compare October 17, 2018 09:06
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Couple of nits. I'll leave the rest to @marten-seemann.

One thing that may be tricky is the following:

  1. Dial.
  2. Listen.
  3. Dial.

Ideally, the second dial would use the source-port specified in the listen. However, with the current code, it may use the source-port from the first dial.

When dialing before we've started listening, I wonder if we should just keep a separate "unspecified" conn (one for IPv6, one for IPv4?). Not sure. I'll let you and @marten-seemann discuss this.

listener.go Show resolved Hide resolved
transport.go Outdated
}

func (c *connManager) GetConnForAddr(network string) (net.PacketConn, error) {
func pickDialConn(conns map[net.PacketConn]int, remoteHostIP net.IP) net.PacketConn {
backup := make([]net.PacketConn, 0, len(conns))
Copy link
Member

Choose a reason for hiding this comment

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

So, this is going to be called a lot (every dial). Let's just set one backup if and only if a backup hasn't been set. We don't really need to collect a list of them and then select the first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, will do.

transport.go Outdated
}
}
c.mu.Lock()
c.ipv4Conns[conn] += 1
Copy link
Member

Choose a reason for hiding this comment

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

What are we using these refcounts for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial thought was not actually close a socket when upper layer call Close(), except the refcount down to 0. But go-reuseport-transport doesn't do this, so I'd like to discuss if it's reasonable.

@cannium
Copy link
Contributor Author

cannium commented Oct 19, 2018

From my test, in current usage pattern, listen comes before dial, but yes, that could be a problem. So it's another trade-off to deal, if it's possible that dial comes before listen, a change is necessary.

@cannium
Copy link
Contributor Author

cannium commented Oct 23, 2018

ping @marten-seemann for review

@Stebalien
Copy link
Member

So it's another trade-off to deal, if it's possible that dial comes before listen, a change is necessary.

It's unusual but completely possible.

transport.go Outdated
c.connIPv4, err = c.createConn(network, "0.0.0.0:0")
})
return c.connIPv4, err
conn := c.pickDialConn(c.ipv4Conns, remoteAddr.IP)
Copy link
Member

Choose a reason for hiding this comment

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

This is going to need some synchronization (we're reading a shared ipv4Conns map and writing a shared c.defaultConn variable.

@cannium cannium closed this Oct 23, 2018
@cannium cannium reopened this Oct 23, 2018
@cannium cannium force-pushed the port-reuse branch 2 times, most recently from 8c01467 to 2dd576b Compare October 23, 2018 10:23
@cannium
Copy link
Contributor Author

cannium commented Oct 23, 2018

After some trial and error, it occurs to me that the right map for connections is conn -> remote addr type, which could also be interpreted as conn -> usage of the conn, so "conn A is used for local peers", "conn B is used for global peers", and "conn C is the default conn, for unspecified peers".

Copy link
Collaborator

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand everything that's going on here, since the code doesn't come with any comments and explanations.

transport.go Show resolved Hide resolved
transport.go Outdated Show resolved Hide resolved
mu sync.Mutex
defaultConn net.PacketConn
// map underhood PacketConn -> connection remote address type(usage of the conn)
ipv4Conns map[net.PacketConn]addrType
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the maps be the other way around, i.e. map[addrType]net.PacketConn?

Copy link
Contributor Author

@cannium cannium Jan 2, 2019

Choose a reason for hiding this comment

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

Then the map would become map[addrType][]net.PacketConn, which is actually a little more complex to handle than current map.

transport.go Outdated
connIPv4Once sync.Once
connIPv4 net.PacketConn
mu sync.Mutex
defaultConn net.PacketConn
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand what the defaultConn is needed for. Can you please explain (and add a comment)?

Copy link
Contributor Author

@cannium cannium Jan 2, 2019

Choose a reason for hiding this comment

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

The behavior is inherited from libp2p/go-reuseport-transport. It tries to use same type of source and destination addresses when possible. For example, both connections bound to 0.0.0.0:1000 and 127.0.0.1:2000 could be used to dial 127.0.0.1:1234, but in this case 127.0.0.1:2000 is preferred, whereas 0.0.0.0:1000 as the default connection, and could be used as a fallback.

transport.go Outdated Show resolved Hide resolved
transport.go Outdated Show resolved Hide resolved
transport.go Outdated Show resolved Hide resolved
if err != nil {
return nil, err
}
conn, err := c.createConn(network, host)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you always creating a new conn when listening? Can't we reuse a conn that was dialed before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned by @Stebalien here:

We can reuse the same port (and even the same underlying OS socket) by calling pc := net.ListenPacket("udp", ":NNNN"). From here, we can send packets to any ip/port with pc.WriteTo(buf, dest) and read incoming packets from any ip/port with pc.ReadFrom(buf).

OS kernel handles it for us.
Also, very rarely we need to listen on ports dynamically.

connManager.mu.Lock()
switch network {
case "udp4":
delete(connManager.ipv4Conns, l.conn)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you delete these connections from the connManager? There might still be outgoing connections running there, which could be reused by other Dials, as well as later calls to Listen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe l.quicListener.Close() would call PacketConn's Close(), so the connection could not be used anymore.

- maintain a default conn
- handle networks other than "udp4", "udp6" when listener.close()
- maintain conn address types
- some refactors
@cannium cannium force-pushed the port-reuse branch 4 times, most recently from e0d65c5 to 6def13e Compare January 2, 2019 07:56
@Stebalien
Copy link
Member

Let's go back to the drawing board with this. Let's keep the code but close the PR so we can spend some time figuring out the right way to proceed. As-is, the current approach isn't going to work. See: #8 (comment)

@Stebalien Stebalien closed this Jan 7, 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.

3 participants