identify: refactor sending of Identify pushes #1984
Conversation
marten-seemann
force-pushed
the
refactor-identify-push-sending
branch
from
64163a2
to
5721c35
Compare
marten-seemann
force-pushed
the
identify-remove-delta
branch
from
5093dc1
to
5095f44
Compare
marten-seemann
force-pushed
the
refactor-identify-push-sending
branch
from
5721c35
to
fba4a51
Compare
marten-seemann
force-pushed
the
refactor-identify-push-sending
branch
4 times, most recently
from
c6d5c8b
to
f5fa030
Compare
p2p/net/mock/mock_test.go
Outdated
| @@ -528,7 +528,7 @@ func TestLimitedStreams(t *testing.T) { | |||
| } | |||
|
|
|||
| wg.Wait() | |||
| if !within(time.Since(before), time.Second*2, time.Second) { | |||
| if !within(time.Since(before), time.Second*3, time.Second) { | |||
There was a problem hiding this comment.
We're now more eager to send an Identify Push: We do it even when we don't know if the peer support push yet (i.e. before the initial identify exchange completes). This means that we now have to fit the Identify message through the (bandwidth-limited) pipe as well, which messes with the timing here.
marten-seemann
force-pushed
the
refactor-identify-push-sending
branch
5 times, most recently
from
63a38de
to
e31c6e9
Compare
marten-seemann
force-pushed
the
refactor-identify-push-sending
branch
3 times, most recently
from
b309b9b
to
7fae446
Compare
|
Fixed (hopefully) all of the flaky tests. This is ready for review now. |
| if err != nil { // connection might have been closed recently | ||
| return | ||
| } | ||
| // TODO: find out if the peer supports push if we didn't have any information about push support |
There was a problem hiding this comment.
Do you want to handle this TODO? Should be as easy as checking the error returned either above or below (or both?)
p2p/protocol/identify/id.go
Outdated
| ids.currentSnapshot.Lock() | ||
| snapshot := ids.currentSnapshot.snapshot | ||
| ids.currentSnapshot.Unlock() | ||
| if !e.Timestamp.Before(snapshot.timestamp) { |
There was a problem hiding this comment.
Who updates e.Timestamp?
There was a problem hiding this comment.
This is now done in sendIdentifyResp. Not sure how to properly test it though.
There was a problem hiding this comment.
A unit test that makes sure we have the timestamp in the connection after doing an identify?
There was a problem hiding this comment.
I'd prefer to test behavior, not implementation specifics.
It's not a long lived Go routine, but this still spawns a Go routine per connection to drive the identify. I think this is okay since it's bounded by the number of connections we have. |
marten-seemann
force-pushed
the
refactor-identify-push-sending
branch
from
86b20f8
to
7fae446
Compare
It's limited by a semaphore, so we only spawn a maximum of 32 Go routines (plus the one that's driving the respective round of Identify Push), no matter how many connections we're actually handling. |
marten-seemann
force-pushed
the
refactor-identify-push-sending
branch
from
7fae446
to
f3fbee4
Compare
Fixes #1965.
This is a major refactor of Identify. The general idea here is:
peerHandler. Adding and removing handlers was not trivial, since we had to deal with (concurrent) disconnect events. Instead of tracking peers, we now track individual connections.IdentifyWaitnot only when we receive the response for the Identify request, but also when we receive an Identify Push. This will be useful once nodes start pushing Identify right after a connection is established (see Tracking Issue: QUIC 0.5-RTT Optimization #1961 why we want this). Since this PR already contains a bunch of changes, this will be done in a followup PR.Fixes #1747. Fixes #1584. Fixes #1523. Fixes #1164. Fixes #1101. Fixes #938.