implement connection reuse #63

marten-seemann · 2019-06-02T12:01:24Z

Fixes #8. Closes #52.

This PR builds on #52, cleans up a lot of code, and makes sure that connections that we're listening on are actually reused for dialing (that was a bug in #52).

The logic is now the following:

For calls to Listen, always create a new connection.
- on 0.0.0.0: call this a "global" connection
- on any other IP: call this a "unicast" connection
When dialing
- if netlink is available (i.e. on Linux), look up the source IP address that the kernel would choose. If we have a unicast connection, use that one
- otherwise, take any global connection (we don't care about the port number)
- if no global connection is available, create one

The listening part of this is still a bit racy, if the user decides to listen on an ephemeral port that was picked for a Dial call before. We should reuse connections when Listen is called after Dial (and the port happens to be the same) as well. Planning to fix this in a separate PR.

As far as I understand, on non-Linux systems, this is effectively equivalent to what we have now: we use the same connection, which is listening on 0.0.0.0, for all outgoing dials. @Stebalien, is that what you described in #8 (comment)? I'd like to hear your feedback on this PR before continuing the work.

This PR does not YET deal with closing connections. The reuseConn is already reference counted, so we can always get the number of QUIC connections that are running on top of this connection. However, we don't do anything once this number reaches 0.
There are two ways we can handle this:

Introduce a callback, and immediately close the connection when the reference counter reaches 0.
Implement a sweeper that runs periodically, and close connections that have been idle for longer than x.

I'm leaning towards 2., since this will make sequential dials (which might happen for reconnects) more efficient.

marten-seemann · 2019-07-27T21:36:09Z

@Stebalien Any thoughts on this PR?

Stebalien · 2019-07-31T22:55:08Z

As far as I understand, on non-Linux systems, this is effectively equivalent to what we have now: we use the same connection, which is listening on 0.0.0.0, for all outgoing dials. @Stebalien, is that what you described in #8 (comment)? I'd like to hear your feedback on this PR before continuing the work.

Sounds like it, I'll read the code.

I'm leaning towards 2., since this will make sequential dials (which might happen for reconnects) more efficient.

I agree.

Stebalien

Logic mostly LGTM.

Stebalien · 2019-08-01T00:38:33Z

reuse.go

+type reuse struct {
+	mutex sync.Mutex
+
+	unicast map[string] /* IP.String() */ map[int] /* port */ *reuseConn


nit: this can just be string(IP).

other nit: do we really want to map port to conn or just map[*reuseConn]struct{}?

It needs to be a map of port to conn, if we ever want to reuse a connection that we dialed on for listening. I admit, this is a rather rare case, but it doesn't cost us anything.

I'm not sure I understand what you mean by string(IP).

libp2p/go-yamux#6 (review)

👍

I'm not sure I understand what you mean by string(IP).

That is, we can convert the raw bytes directly to a string rather than formatting as "xxx.xxx.xxx.xxx"). It would make lookups zero-allocation (mymap[string(someByteArray)] doesn't allocate).

That's great! I wasn't aware that net.IP is just a []byte.

Great in theory, that is. It looks like there's no obvious way to normalize IP addresses, and it's failing in our tests.

I managed to reproduce the issue locally, the IP is 192.168.46.226.
The kernel resolves this to: net.IP{0xc0, 0xa8, 0x2e, 0xe2}.
After listening, the local address is: net.IP{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xff, 0xff, 0xc0, 0xa8, 0x2e, 0xe2}

Normalizing this looks straightforward in this case, however, I'm a bit wary of pitfalls when using IPv6 addresses. Map lookups will hardly be the most expensive operation when starting a new QUIC listener / dialer, so I think we'll be fine for now with leaving this as a net.IP.String().

reuse.go

Stebalien · 2019-08-01T01:06:49Z

reuse.go

+		return nil, err
+	}
+	rconn := newReuseConn(conn)
+	r.global[conn.LocalAddr().(*net.UDPAddr).Port] = rconn


Ideally, we'd mark this as a fallback connection so we can stop using it when we get a real global connection.

I'm not sure I understand. Dial will use new connections if they're available, so this effective is already a fallback, isn't it?

Events:

We dial, creating the fallback.

We listen on a specific port.

We dial again.

At step 3, we may use the connection from step 1 or step 3. Ideally, we'd use the connection from step 2.

I guess you mean that ideally, we'd use the connection from step 2.

I'm going to merge this PR now, so we don't have to go through another round of review on this one. This also touches on reusing dialed connections for listening.

(yes, sorry)

reuse.go

Stebalien · 2019-08-05T12:48:06Z

transport.go

+	go func() {
+		<-sess.Context().Done()
+		pconn.DecreaseCount()
+	}()


We can leave this for now but we may want to simply wrap close in the future. Leaving an extra goroutine open can get expensive.

Not every connection is closed by calling Close. Only the peer that's actively closing the connection calls Close. The other peer will just receive a connection failure. Or the connection might time out, in which case nobody calls Close at all.

Stebalien

LGTM (but still read my comments)

marten-seemann requested a review from Stebalien June 2, 2019 12:01

This was referenced Jun 14, 2019

Improve network reliability berty/berty#1235

Closed

feat(network): update quic to 0.1.2 (reuse port) berty/berty#1237

Merged

Stebalien mentioned this pull request Jul 23, 2019

QUIC's roadmap to stable libp2p/go-libp2p#688

Closed

5 tasks

Stebalien suggested changes Aug 1, 2019

View reviewed changes

marten-seemann force-pushed the reuseport branch 3 times, most recently from 8ef00ef to 074b517 Compare August 5, 2019 11:19

lnykww and others added 2 commits August 5, 2019 18:37

reuse listening connections for dialing

79c61fb

use a single handle for each reuse

01a06cd

marten-seemann force-pushed the reuseport branch from 074b517 to 01a06cd Compare August 5, 2019 11:37

Stebalien reviewed Aug 5, 2019

View reviewed changes

Stebalien approved these changes Aug 5, 2019

View reviewed changes

marten-seemann force-pushed the reuseport branch from 94f024a to 01a06cd Compare August 6, 2019 08:30

marten-seemann marked this pull request as ready for review August 6, 2019 08:31

marten-seemann merged commit cd4717d into master Aug 6, 2019

This was referenced Aug 6, 2019

close unused connections #70

Closed

quic: listen on connections that we dialed from before libp2p/go-libp2p#1534

Closed

Stebalien deleted the reuseport branch August 6, 2019 14:16

marten-seemann mentioned this pull request Aug 7, 2019

implement a garbage-collector for unused reuse connections #73

Merged

sukunrt mentioned this pull request Apr 27, 2023

quic: prioritise listen connections for reuse libp2p/go-libp2p#2262

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement connection reuse #63

implement connection reuse #63

marten-seemann commented Jun 2, 2019 •

edited

Loading

marten-seemann commented Jul 27, 2019

Stebalien commented Jul 31, 2019

Stebalien left a comment

Stebalien Aug 1, 2019

Stebalien Aug 1, 2019

marten-seemann Aug 5, 2019

marten-seemann Aug 5, 2019

Stebalien Aug 5, 2019

marten-seemann Aug 6, 2019

marten-seemann Aug 6, 2019

Stebalien Aug 1, 2019 •

edited

Loading

marten-seemann Aug 5, 2019

Stebalien Aug 5, 2019 •

edited

Loading

marten-seemann Aug 6, 2019 •

edited

Loading

Stebalien Aug 6, 2019

Stebalien Aug 5, 2019

marten-seemann Aug 6, 2019

Stebalien left a comment

implement connection reuse #63

implement connection reuse #63

Conversation

marten-seemann commented Jun 2, 2019 • edited Loading

marten-seemann commented Jul 27, 2019

Stebalien commented Jul 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien Aug 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien Aug 5, 2019 • edited Loading

Choose a reason for hiding this comment

marten-seemann Aug 6, 2019 • 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

Stebalien left a comment

Choose a reason for hiding this comment

marten-seemann commented Jun 2, 2019 •

edited

Loading

Stebalien Aug 1, 2019 •

edited

Loading

Stebalien Aug 5, 2019 •

edited

Loading

marten-seemann Aug 6, 2019 •

edited

Loading