Fix simultaneous dials #250

vyzo · 2021-03-30T14:17:52Z

Alternative to #246

This implements a join/split algorithm for synchronizing simultaneous dials through a dial worker.

makes TestDialBackoff happy

vyzo · 2021-03-30T18:47:45Z

TestConnectionGating appears to be flaky, it intermittently fails on the base branch as well.
Opened issue #251.

Stebalien

In terms of testing, the codecov comments are actually pretty useful.

dial_sync.go

swarm_dial.go

dial_sync.go

swarm_dial.go

Stebalien · 2021-03-30T21:45:27Z

swarm_dial.go

+			}
+
+			// the request has some pending or new dials, track it and schedule new dials
+			reqno++


nit: we can also just use the request object as a key, right?

Not really. We need to pack request ids in the addrDial struct and then check to see if they have been complete (removed from the tracking).
If we used the request object as key we'd have to copy the requests all over the place and then do extra management stuff to keep them up-to-date.

What's wrong with using an integer? It doesn't cost anything.

It works, it's just something else to track.

If we used the request object as key we'd have to copy the requests all over the place and then do extra management stuff to keep them up-to-date.

Can't we use the pointers? No need to copy anything and should be the same size as an int.

(obviously not a huge deal)

Well, we could use a req pointer but then we'd have to reify the struct and move it to the heap -- which is strictly worse than using integers, because there is one more thing for the gc to track. Not a huge impact though.

I would agree with tracking overhead, but the way we use the integers is really simple,
The reqno is incremented at exactly one place, right before we add to the tracking map, so there is very little mental overhead involved.

I am really inclined to leave it as is, unless you feel very strongly about it.

I'm happy to leave it as it is, but I think I'm confused about something. The request is allocated here:

https://github.com/libp2p/go-libp2p-swarm/pull/250/files#diff-3025a253202bbb3d70750d06692c51f9361993ecafcce2bddbebd5508337faaaR415-R419

and we're storing it by reference in the requests map. It should be possible to just change the requests map to map[*pendRequest]struct{}. and ad.requests to []*pendRequest.

Actually... I think we can even get rid of the requests map.

We can't that easily, we will have to track what request is still active.
That's the main reason for the indirection, and that's why I like the integers as pointers approach -- it forces you to check for validity when you want to use it.

swarm_dial.go

Stebalien · 2021-03-30T22:51:15Z

First pass review. I like the approach.

…awning the loop

i want to use binary literals; technically only requires 1.13, but let's not be ancient

… dial error

for consistency with the old dialer behaviour.

we might get more connections because simultaneous dials can succeed and we have both TCP and QUIC addrs by default

Stebalien

Looking good but I want to take a closer look when I'm more awake (got a bit distracted today).

However, can we please break the batching into a separate PR? This PR is already very large and the batching going to trigger another few rounds of discussion.

Stebalien · 2021-04-01T08:19:01Z

swarm_dial.go

+			}
+
+			// the request has some pending or new dials, track it and schedule new dials
+			reqno++


(obviously not a huge deal)

vyzo · 2021-04-01T08:57:57Z

However, can we please break the batching into a separate PR? This PR is already very large and the batching going to trigger another few rounds of discussion.

Sure, I'll do that after fixing the test package situation.

… a dial backoff occurs

vyzo · 2021-04-01T09:25:57Z

Removed the dial batching -- follow up pr in #252.

so that we exercise the dialWorker dial to self error path

they work with private data types, so there is no point in having them public

Stebalien

Nice! We'll have to deploy this on infra to test but we might as well merge it now then back out if we find an issue.

swarm_dial.go

Stebalien · 2021-04-01T19:31:28Z

swarm_dial.go

+			}
+
+			// the request has some pending or new dials, track it and schedule new dials
+			reqno++


I'm happy to leave it as it is, but I think I'm confused about something. The request is allocated here:

https://github.com/libp2p/go-libp2p-swarm/pull/250/files#diff-3025a253202bbb3d70750d06692c51f9361993ecafcce2bddbebd5508337faaaR415-R419

and we're storing it by reference in the requests map. It should be possible to just change the requests map to map[*pendRequest]struct{}. and ad.requests to []*pendRequest.

Actually... I think we can even get rid of the requests map.

Stebalien · 2021-04-01T19:35:06Z

swarm_dial.go

+	return goodAddrs, nil
+}
+
+func (s *Swarm) mergeDialContexts(a, b context.Context) context.Context {


I really don't like this but let's punt on making any more changes till a followup.

ok, fair enough.

swarm_dial.go

Stebalien · 2021-04-01T19:41:00Z

swarm_dial.go

+// NonWS > WS
+// Private > Public
+// UDP > TCP
+func (s *Swarm) rankAddrs(addrs []ma.Multiaddr) []ma.Multiaddr {


Is this really any better?

Hm. Yes it is.

It's consistent, the code is not a jumble mess of ifs, and it is extensible...

Stebalien · 2021-04-01T19:43:51Z

swarm_dial.go

+// Private > Public
+// UDP > TCP
+func (s *Swarm) rankAddrs(addrs []ma.Multiaddr) []ma.Multiaddr {
+	addrTier := func(a ma.Multiaddr) (tier int) {


nit: we could make this more generic by ranking according to a list of functions.

I.e.:

var rankFunctions = []func(ma.Multiaddr) bool{} func rank(m ma.Multiaddr) int { rank := 0 for i, f := range rankFunctions { if f(m) { rank |= (1<<i) } } return rank }

yes, we could -- let's keep this in mind for subsequent iterations; it's just 4 bits now.

Stebalien · 2021-04-01T19:59:21Z

@vyzo merge when ready. This is a great step forward.

vyzo added 2 commits March 30, 2021 17:09

deps: update go-libp2p-core

8930f29

implement dial worker for synchronizing simultaneous dials

9d50a8c

vyzo requested a review from Stebalien March 30, 2021 14:17

vyzo mentioned this pull request Mar 30, 2021

Fix simultaneous dials #246

Closed

vyzo changed the title ~~Fix simultaneous dial~~ Fix simultaneous dials Mar 30, 2021

vyzo added 3 commits March 30, 2021 17:46

adjust next dial delays

201a8d1

fix dial_sync tests

2e742fb

clear address dial when they fail because of backoff

5900641

makes TestDialBackoff happy

vyzo marked this pull request as ready for review March 30, 2021 18:36

Stebalien reviewed Mar 30, 2021

View reviewed changes

Stebalien suggested changes Mar 30, 2021

View reviewed changes

swarm_dial.go Outdated Show resolved Hide resolved

swarm_dial.go Outdated Show resolved Hide resolved

swarm_dial.go Outdated Show resolved Hide resolved

Stebalien suggested changes Mar 30, 2021

View reviewed changes

swarm_dial.go Outdated Show resolved Hide resolved

Stebalien mentioned this pull request Mar 30, 2021

Enable the websocket listener by default ipfs/kubo#5251

Open

vyzo added 7 commits March 31, 2021 12:27

nuke incref, it's useless

de528f1

make dialWorker return an error for self dials and responsible for sp…

2ee7bf0

…awning the loop

don't use a goroutine for the actual dial

0510dcb

bump go version to 1.15

96723d1

i want to use binary literals; technically only requires 1.13, but let's not be ancient

batch dials together, rework address ranking

bbd0a01

tune down batch dial delays

a6c2838

use a timer instead of time.After

7ccf58e

vyzo requested a review from Stebalien March 31, 2021 13:44

vyzo added 7 commits March 31, 2021 17:46

kill dial jump delays

0fc0ade

add TestDialExistingConnection

12a0cdb

do a last ditch check for acceptable connections before dispatching a…

e7b6af6

… dial error

merge dial contexts where possible

580a818

add TestDialSimultaneousJoin test

699b4d1

don't add backoff if we have successfully connected

b67b736

for consistency with the old dialer behaviour.

fix TestConnectednessCorrect

0538806

we might get more connections because simultaneous dials can succeed and we have both TCP and QUIC addrs by default

vyzo added 2 commits March 31, 2021 23:04

don't store the active dial if it errors while starting the worker

acc35e8

add TestSelfDial

27f6c39

Stebalien reviewed Apr 1, 2021

View reviewed changes

swarm_dial.go

}

// the request has some pending or new dials, track it and schedule new dials

reqno++

Copy link

Member

Stebalien Apr 1, 2021

Choose a reason for hiding this comment

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

(obviously not a huge deal)

vyzo added 3 commits April 1, 2021 12:13

make DialRequest and DialResponse private

1624828

add comment about the necessity of removing the address tracking when…

13d3556

… a dial backoff occurs

remove dial batching

4a69fa2

vyzo mentioned this pull request Apr 1, 2021

Dial batching #252

Closed

vyzo force-pushed the fix/simultaneous-dial branch from 967e89f to 01014c8 Compare April 1, 2021 09:44

add new TestDialSelf

43b0382

so that we exercise the dialWorker dial to self error path

vyzo force-pushed the fix/simultaneous-dial branch from 01014c8 to 43b0382 Compare April 1, 2021 09:47

make DialWorkerFunc, NewDialSync private

17bc04b

they work with private data types, so there is no point in having them public

Stebalien approved these changes Apr 1, 2021

View reviewed changes

Stebalien reviewed Apr 1, 2021

View reviewed changes

vyzo added 3 commits April 1, 2021 23:11

rename dialWorker to startDialWorker

be3e940

make addr utility funcs standalone and not exported

df0ab8b

make IsFdConsumingAddr a standalone utility func

084fffe

vyzo merged commit fe4ee0b into fix/direct-connect Apr 1, 2021

vyzo deleted the fix/simultaneous-dial branch April 1, 2021 20:36

vyzo mentioned this pull request Apr 1, 2021

New Dialer #243

Merged

anacrolix mentioned this pull request May 2, 2021

Addresses with trailing peer IDs get filtered #111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix simultaneous dials #250

Fix simultaneous dials #250

vyzo commented Mar 30, 2021 •

edited

Loading

vyzo commented Mar 30, 2021

Stebalien left a comment

Stebalien Mar 30, 2021

vyzo Mar 31, 2021

Stebalien Apr 1, 2021

Stebalien Apr 1, 2021

vyzo Apr 1, 2021 •

edited

Loading

Stebalien Apr 1, 2021

vyzo Apr 1, 2021

Stebalien commented Mar 30, 2021

Stebalien left a comment

Stebalien Apr 1, 2021

vyzo commented Apr 1, 2021 •

edited

Loading

vyzo commented Apr 1, 2021

Stebalien left a comment

Stebalien Apr 1, 2021

Stebalien Apr 1, 2021

vyzo Apr 1, 2021

Stebalien Apr 1, 2021

Stebalien Apr 1, 2021

vyzo Apr 1, 2021

Stebalien Apr 1, 2021

Stebalien Apr 1, 2021

vyzo Apr 1, 2021

Stebalien commented Apr 1, 2021

Fix simultaneous dials #250

Fix simultaneous dials #250

Conversation

vyzo commented Mar 30, 2021 • edited Loading

vyzo commented Mar 30, 2021

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

vyzo Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Mar 30, 2021

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented Apr 1, 2021 • edited Loading

vyzo commented Apr 1, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Apr 1, 2021

vyzo commented Mar 30, 2021 •

edited

Loading

vyzo Apr 1, 2021 •

edited

Loading

vyzo commented Apr 1, 2021 •

edited

Loading