react to incoming events #65
Conversation
autonat.go
Outdated
| timerRunning = true | ||
| } | ||
| } | ||
|
|
||
| // scheduleProbe calculates when the next probe should be scheduled for, | ||
| // and launches it if that time is now. | ||
| func (as *AmbientAutoNAT) scheduleProbe() time.Duration { | ||
| func (as *AmbientAutoNAT) scheduleProbe(peer peer.ID) time.Duration { |
There was a problem hiding this comment.
This function is starting to do way too many things, only one of which is scheduling a probe.
There was a problem hiding this comment.
I agree with @Stebalien. I tried reading it to understand what it does and it does take some effort.
|
Given that this isn't critical to land immediately, I'd like to think through the right way to do this. The old code was highly geared towards periodic probing and this feels like we're trying to bolt on probing specific peers. I'd consider making this two separate code paths. |
|
restructured the functions to make things more straight forward, hopefully addressing the main confusion. (although there's a bit of 2 flows happening in the main eventloop + timer scheduling functions still) |
autonat.go
Outdated
| timerRunning = true | ||
| } | ||
| } | ||
|
|
||
| // scheduleProbe calculates when the next probe should be scheduled for, | ||
| // and launches it if that time is now. | ||
| func (as *AmbientAutoNAT) scheduleProbe() time.Duration { | ||
| func (as *AmbientAutoNAT) scheduleProbe(peer peer.ID) time.Duration { |
There was a problem hiding this comment.
I agree with @Stebalien. I tried reading it to understand what it does and it does take some effort.
willscott
force-pushed
the
feat/incomingevent
branch
from
69b7e9c
to
aac6263
Compare
|
this has been waiting for review for a while, @Stebalien / @aarshkshah1992 - will make startup quite a bit faster. |
|
This still doesn't make much sense to me. Let's re-think this from the ground up:
|
|
Did a fairly major re-factor of this PR.
|
autonat.go
Outdated
| addrUpdatedChan := subAddrUpdated.Out() | ||
| defer subAddrUpdated.Close() | ||
|
|
||
| subIDOccured, _ := as.host.EventBus().Subscribe(new(event.EvtPeerIdentificationCompleted)) |
There was a problem hiding this comment.
Also, we can probably just subscribe to both events at the same time.
autonat.go
Outdated
| // peer identification occured. | ||
| case idev := <-IDChan: | ||
| peer = idev.(event.EvtPeerIdentificationCompleted).Peer | ||
| if s, err := as.host.Peerstore().SupportsProtocols(peer, AutoNATProto); err != nil || len(s) > 0 { |
There was a problem hiding this comment.
If currentStatus is Unknown? We don't want to probe everyone.
|
|
||
| for { | ||
| select { | ||
| // new connection occured. | ||
| // new inbound connection. | ||
| case conn := <-as.inboundConn: |
There was a problem hiding this comment.
We should also check "is the remote addr public". We can do this in a followup PR.
| @@ -321,13 +376,13 @@ func (as *AmbientAutoNAT) probe(pi *peer.AddrInfo) { | |||
| } | |||
There was a problem hiding this comment.
OT: above: we need to select on the service context in case we shutdown.
| if !as.config.dialPolicy.skipPeer(info.Addrs) { | ||
| addrs = append(addrs, info) | ||
| candidates = append(candidates, p) | ||
| } |
There was a problem hiding this comment.
nit: use if ... { continue } for all checks, then append at the end. It makes this kind of logic a bit easier to read.
|
For "OT" things, feel free to file issues or address in a followup. |
largely addresses #57