Quic reuseport #52

lnykww · 2019-03-20T04:30:00Z

Fixes #8
Implementation：#8 (comment)

I am not sure where the source select library should be placed, so I created a repository on my github to put this code. If it is not appropriate, please let me know where to put this code.

Stebalien

Nice!

This will need a bunch of tests. Especially around closing listeners, connections, picking the right one, etc. They don't have to be that fancy, but we should test unspecified versus localhost, etc.
We can leave your dep where it is or adopt it into the libp2p org, I'm happy to do either. I'd also like to try adding windows support before shipping this but that shouldn't actually be too difficult (we can work on that after we finish up with this PR).

The current design (as well as the current code, for that matter) is nice and simple but, as far as I can tell, it doesn't allow us to actually close the underlying udp sockets when we no longer need them (because we can't know if we no longer need them). We may want to reference count on create/close.

If we're going to be wrapping connections anyways, we should consider a manager of the form:

type Manager interface {
    // Dial dials the remote addr, reusing an existing socket if possible.
    Dial(network string, addr *net.UDPAddr) (net.PacketConn, error)
    // Listen listens on the given address.
    Listen(network string, addr *net.UDPAddr) (net.PacketConn, error)
}

Where Dial/Listen return connections wrapped in a reference counter.

Closing any connection (listen/dial) would decrement the reference count. When it hits zero, the underlying connection would be closed.
Closing a listener would also remove the listener from the list of open listeners.

That way, we no longer need to manually register/unregister connections. The connection manager just does everything for us.

Note: I don't feel strongly about this interface and, IMO, the current interface is just fine. My main concern is reference counting.

If you'd like, I'd be fine punting reference counting given that it's an existing issue.

Stebalien · 2019-03-21T01:28:11Z

transport.go

@@ -66,12 +129,32 @@ func (c *connManager) createConn(network, host string) (net.PacketConn, error) {
 	return net.ListenUDP(network, addr)
 }

+func (c *connManagers) AddListener(network string, addr *net.UDPAddr, conn net.PacketConn) error {
+	if connManager, ok := c.managers[network]; ok {
+		return connManager.AddListener(network, addr, conn)


Nit: Feeding the network through here just to be thrown away seems funny. We don't have to use the exact same interface for the connManager and the connManagers.

(although I don't mind much either way)

Stebalien · 2019-03-21T01:28:35Z

listener.go

@@ -49,10 +54,12 @@ func newListener(addr ma.Multiaddr, transport tpt.Transport, localPeer peer.ID,
 	}
 	return &listener{
 		quicListener:   ln,
-		transport:      transport,
+		t:              t,


Let's just keep this as transport.

Stebalien · 2019-03-21T01:34:01Z

listener.go

@@ -39,6 +40,10 @@ func newListener(addr ma.Multiaddr, transport tpt.Transport, localPeer peer.ID,
 	if err != nil {
 		return nil, err
 	}
+	err = t.connManagers.AddListener(lnet, laddr, conn)
+	if err != nil {


Do we need to close the connection?

Stebalien · 2019-03-21T01:34:27Z

listener.go

 	privKey        ic.PrivKey
 	localPeer      peer.ID
 	localMultiaddr ma.Multiaddr
+	udpAddr        *net.UDPAddr
+	network        string


I'd mildly prefer to just re-parse the multiaddr on close (storing duplicate information bugs me).

lnykww · 2019-03-21T12:15:16Z

Nice!

This will need a bunch of tests. Especially around closing listeners, connections, picking the right one, etc. They don't have to be that fancy, but we should test unspecified versus localhost, etc.

Test Added.

We can leave your dep where it is or adopt it into the libp2p org, I'm happy to do either. I'd also like to try adding windows support before shipping this but that shouldn't actually be too difficult (we can work on that after we finish up with this PR).

Adopt it into the libp2p org will be better.

The current design (as well as the current code, for that matter) is nice and simple but, as far as I can tell, it doesn't allow us to actually close the underlying udp sockets when we no longer need them (because we can't know if we no longer need them). We may want to reference count on create/close.

If we're going to be wrapping connections anyways, we should consider a manager of the form:
type Manager interface {
    // Dial dials the remote addr, reusing an existing socket if possible.
    Dial(network string, addr *net.UDPAddr) (net.PacketConn, error)
    // Listen listens on the given address.
    Listen(network string, addr *net.UDPAddr) (net.PacketConn, error)
}
Where Dial/Listen return connections wrapped in a reference counter.

Closing any connection (listen/dial) would decrement the reference count. When it hits zero, the underlying connection would be closed.

Closing a listener would also remove the listener from the list of open listeners.

That way, we no longer need to manually register/unregister connections. The connection manager just does everything for us.

Note: I don't feel strongly about this interface and, IMO, the current interface is just fine. My main concern is reference counting.

If you'd like, I'd be fine punting reference counting given that it's an existing issue.

Used referenced connection now.

@Stebalien I have updated the code with your review opinoin. Please for a second review.

Stebalien · 2019-03-21T20:40:15Z

reuse.go

+	"net"
+	"sync"
+
+	srcs "github.com/lnykww/go-src-select"


I've forked this to github.com/libp2p/go-src-select I can also delete that and we can transfer it if you'd like.

Could you add a LICENSE? Ideally Apache 2.0 + MIT. Take a look at the COPYRIGHT, LICENSE-MIT, and LICENSE-APACHE files in ipfs/go-ds-crdt@d51c9f1.

Note: This project is still copyright you (well, you and whoever else ends up contributing to it).

Stebalien · 2019-03-21T20:46:13Z

reuse.go

+	srcs "github.com/lnykww/go-src-select"
+)
+
+type ReuseConn struct {


I'd keep all these types internal for now.

Stebalien · 2019-03-21T20:48:13Z

reuse.go

+	return reuseConn.Ref()
+}
+
+func (r *Reuse) dial(network string, raddr *net.UDPAddr) (net.PacketConn, error) {


We can return a *ReuseConn here. Then we can drop reuseConn and just call Ref directly.

Stebalien · 2019-03-21T20:51:22Z

reuse.go

+	}
+}
+
+func (rc *ReuseConn) Ref() error {


Probably simpler just to return a boolean true for success.

Stebalien · 2019-03-21T20:53:48Z

reuse.go

+		return nil, err
+	}
+	// we are reuse a conn, reference it
+	if err = r.reuseConn(conn); err != nil {


I believe this will double-ref if we end up creating a new global connection.

We probably need a loop (either inside dial out outside). Otherwise, we could try to use the connection but fail because some other thread decided it didn't want it. In that case, we should just open a new connection.

I believe this will double-ref if we end up creating a new global connection.

I have found this problem before. But we create a listen socket as a global socket,
it means that if we don't want to use it, we need close it ourself in this module,
rather than the user in other module. So when it was created, the reference should be 1.

We probably need a loop (either inside dial out outside). Otherwise, we could try to use the connection but fail because some other thread decided it didn't want it. In that case, we should just open a new connection.

Ok, i will add a retry for that.

I have found this problem before. But we create a listen socket as a global socket,
it means that if we don't want to use it, we need close it ourself in this module,
rather than the user in other module.

My thinking was that, when the last connection using the global connection closes, we could close the global connection (reopening a new one when necessary).

My thinking was that, when the last connection using the global connection closes, we could close the global connection (reopening a new one when necessary).

This may cause us to create new socket frequently.

If we're repeatedly closing all connections. Also note: this will only happen if we aren't listening on a relevant ip address.

My worry here is that we're leaking a socket. In theory, a user should be able to spin up/down new libp2p instances without leaking anything.

Yes, you are right. It does not happen very often. I will do that.

Stebalien · 2019-03-21T21:03:06Z

So, I realized something annoying. quic-go correctly won't close connections it didn't create. @marten-seemann is this something that could be made configurable? I.e., some way to "give" a connection to QUIC? This patch is introducing connection refcounting but nothing will ever call Close.

@lnykww otherwise, we'll have to stash the pconn in the go-libp2p-quic-transport.conn and close it manually.

Let's wait for @marten-seemann to chime in here before we move forward.

marten-seemann

Some stylistic comments. Will do a full review later.

marten-seemann · 2019-03-25T16:41:31Z

listener.go

@@ -35,7 +34,7 @@ func newListener(addr ma.Multiaddr, transport tpt.Transport, localPeer peer.ID,
 	if err != nil {
 		return nil, err
 	}
-	conn, err := net.ListenUDP(lnet, laddr)
+	conn, err := t.connManagers.Listen(lnet, laddr)


Please don't access unexported fields of other structs.

I should have been more specific. Please don't access any fields of other structs.

marten-seemann · 2019-03-25T16:42:00Z

listener.go

+	if err != nil {
+		return err
+	}
+	err = l.transport.connManagers.Close(lnet, laddr)


marten-seemann · 2019-03-25T16:42:50Z

reuse.go

+
+type ReuseConn struct {
+	net.PacketConn
+	lock sync.Mutex


Convention: This should be called mutex.

marten-seemann · 2019-03-25T16:43:19Z

reuse.go

+	return &ReuseConn{
+		PacketConn: conn,
+		ref:        1,
+		lock:       sync.Mutex{},


No need to initialize a mutex. The default value works fine.

marten-seemann · 2019-03-25T16:44:21Z

reuse.go

+type ReuseConn struct {
+	net.PacketConn
+	lock sync.Mutex
+	ref  int


Why is ref an int, if it only ever takes the value 0 or 1?

ref indicates how many connection was use it. and its value is not only 0 or 1.

marten-seemann · 2019-03-25T16:44:49Z

reuse.go

+}
+
+type Reuse struct {
+	lock           sync.Mutex


marten-seemann · 2019-03-26T05:59:57Z

reuse.go

+// dialGlobal get the global random port socket, if not exist, create
+// it first.
+func (r *Reuse) dialGlobal(network string) (*reuseConn, error) {
+	var err error


This err is unused.

marten-seemann · 2019-03-26T06:01:21Z

reuse.go

+
+func NewReuse() *Reuse {
+	return &Reuse{
+		unicast:    make(map[string]map[int]*reuseConn),


I assume the int should be the port number, right? This will need documentation here.

marten-seemann · 2019-03-26T06:04:58Z

reuse.go

+			r.unicast[laddr.IP.String()] = make(map[int]*reuseConn)
+		}
+		if _, ok := r.unicast[laddr.IP.String()][laddr.Port]; ok {
+			conn.Close()


Why are we closing the connection that we just created a few lines above? Can't we avoid creating it in the first place?

emm, I think we need not to check if we has already listening on an address. Because when we call ListenUDP, the kernel will do that.

marten-seemann · 2019-03-26T06:06:41Z

reuse.go

+	}
+}
+
+func (rc *reuseConn) Ref() bool {


Things will become a lot clearer if you introduce a function to tell if rc.ref > 0 and another function to increase the value.

marten-seemann · 2019-03-26T06:07:20Z

reuse.go

+	}
+
+	rconn := NewReuseConn(conn)
+	_ = rconn.Ref()


This should not be necessary any more with the refactoring I suggested for the ref counter.

it is a bug here. it need not to increase the ref. Because of NewReuseConn already set the ref to 1. Will fix it.

marten-seemann · 2019-03-26T06:08:13Z

reuse.go

+	r.mutex.Lock()
+	defer r.mutex.Unlock()
+
+	switch {


This switch statement would greatly benefit from comments explaining what's going on here.

marten-seemann · 2019-03-26T06:10:15Z

reuse.go

+	defer r.mutex.Unlock()
+	switch {
+	case addr.IP.IsUnspecified():
+		for index, conn := range r.unspecific {


Can we use a map for r.unspecific? Looping over a slice and then deleting values from the middle of it doesn't look very clean.

marten-seemann · 2019-03-26T06:11:57Z

reuse.go

+				r.unspecific = append(r.unspecific[:index], r.unspecific[index+1:]...)
+				return nil
+			}
+		}


What if the connection is not found in the slice? We should handle that case and return an error.
The code will probably become more readable if you use an if instead a switch.

2. Del duplicate check of the unicast and uspecific map. Let kernel help us to check if it is already listening on. 3. Make code clean.

Stebalien · 2019-04-04T07:23:16Z

@marten-seemann can you comment on:

So, I realized something annoying. quic-go correctly won't close connections it didn't create. @marten-seemann is this something that could be made configurable? I.e., some way to "give" a connection to QUIC? This patch is introducing connection refcounting but nothing will ever call Close.

?

marten-seemann · 2019-04-04T07:31:37Z

This is behaving exactly as intended. If you're passing a connection to quic-go, either in quic.Listen or quic.Dial, you can reuse the same connection for multiple connections, and quic-go will handle the multipling. quic-go will therefore never close this connection, it's your responsibility to do this.
If you use quic.ListenAddr or quic.DialAddr, quic-go creates a new connection just for this one purpose, and closes this connection as soon as it's done with it.

Stebalien · 2019-04-04T07:42:25Z

I understand, I'm asking if we could have some way to "give" the connection to quic-go. Alternatively, we can handle the reference counting from the go-libp2p-transport Conn.

marten-seemann · 2019-04-04T07:44:50Z

The point is that quic-go doesn't know when it's ok to close the connection. Only the user knows when it's done quic.Dialing on the connection. Why is this a problem here in the first place?

Stebalien · 2019-04-04T08:00:31Z

The current patch refcounts connections, incrementing the refcount whenever it hands one to quic.

(Note: I'm asking if some kind of "owned" flag would be a reasonable addition to quic-go, I know it won't work at the moment)

marten-seemann · 2019-04-04T08:20:10Z

I'm sorry, I'm afraid I don't understand the problem you're trying to solve. When and why do you want to close a UDP connection?

Stebalien · 2019-04-04T08:59:39Z

When we, e.g., close a listener. We should be able to spin up a bunch of libp2p nodes in as single process and then tear them down without leaking anything.

marten-seemann · 2019-04-04T09:05:37Z

Then we need to close the underlying connections in the same step as we close the listeners. Note that this will also kill all outgoing connections we might have use the packet conn for.

Stebalien · 2019-04-04T09:08:35Z

That's why this is doing ref-counting.

lnykww · 2019-04-22T10:20:00Z

I'm sorry, I'm afraid I don't understand the problem you're trying to solve. When and why do you want to close a UDP connection?

@marten-seemann For creating a new libp2p instance, we might create a new global conn. When we close the libp2p instance, we need to close the global conn to prevent the connection from leaking.

The point is that quic-go doesn't know when it's ok to close the connection. Only the user knows when it's done quic.Dialing on the connection. Why is this a problem here in the first place?

@Stebalien Did this means that we need close the connection ourselves?

Stebalien suggested changes Mar 21, 2019

View reviewed changes

lnykww force-pushed the master branch from 7f10137 to 5120e5c Compare March 21, 2019 10:48

reuse port

6166a12

lnykww force-pushed the master branch from 5120e5c to 6166a12 Compare March 21, 2019 12:13

Stebalien reviewed Mar 21, 2019

View reviewed changes

marten-seemann reviewed Mar 25, 2019

View reviewed changes

lnykww added 4 commits March 26, 2019 10:26

Code style fix

3e69859

add dial retry if reference failed

ef5ec77

add reuse retry and close global connection when not used

4fa4fd4

delete connGlobalOnce

7200054

marten-seemann suggested changes Mar 26, 2019

View reviewed changes

lnykww added 2 commits March 26, 2019 15:47

1. Add more comment.

a109052

2. Del duplicate check of the unicast and uspecific map. Let kernel help us to check if it is already listening on. 3. Make code clean.

add the get method of connmanagers

2b2859c

marten-seemann mentioned this pull request Jun 2, 2019

implement connection reuse #63

Merged

marten-seemann closed this in #63 Aug 6, 2019

Quic reuseport #52

Quic reuseport #52

Conversation

lnykww commented Mar 20, 2019

Stebalien left a 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

lnykww commented Mar 21, 2019

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

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

Stebalien commented Mar 21, 2019

marten-seemann left a 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

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

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

Stebalien commented Apr 4, 2019

marten-seemann commented Apr 4, 2019

Stebalien commented Apr 4, 2019

marten-seemann commented Apr 4, 2019

Stebalien commented Apr 4, 2019

marten-seemann commented Apr 4, 2019

Stebalien commented Apr 4, 2019

marten-seemann commented Apr 4, 2019

Stebalien commented Apr 4, 2019

lnykww commented Apr 22, 2019 • edited Loading

lnykww commented Apr 22, 2019 •

edited

Loading