Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

feat: lower timeout for local address dials #70

Merged
merged 1 commit into from
Jun 17, 2018

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Jun 7, 2018

Part of libp2p/go-libp2p#1553
I didn't add ipv6 to the filters as nodes don't seem to announce the link-local addresses (or we filter them out elsewhere)

Tried this locally, seems to help a bit (limiter no longer fills with local addresses)

  • Is there a better place for address matching logic?

@ghost ghost assigned magik6k Jun 7, 2018
@ghost ghost added the status/in-progress In progress label Jun 7, 2018
swarm.go Outdated
@@ -26,6 +26,11 @@ import (
// protocol selection as well the handshake, if applicable.
var DialTimeout = 60 * time.Second

// DialTimeoutLocal is the maximum duration a Dial to local network address
// is allowed to take. This only includes establishing the raw network
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this dial timeout will also include negotiating the security protocol etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, right

@Stebalien
Copy link
Member

Something like what you're doing here should work as an interum solution. As a long term solution, I'd like to more generally track expected RTTs.

I wonder if we could:

  1. Have some kind of RTT tracking service.
  2. Expose this service to transports. We can do this by feeding the service through with the context when dialing or by passing it into the transports when constructing them (as we do with the upgrader). Personally, I prefer the first option.

When dialing, services would:

  1. Calculate timeouts based on expected RTTs reported by this service.
  2. Report actual RTTs back to this service.

@whyrusleeping
Copy link
Contributor

@magik6k heres the tool i've been using to debug this (and generate those dial logs): https:/libp2p/dht-utils/blob/master/query-test/main.go

(sorry no docs, you might have to gx-go uw in go-ipfs for it to build)

@whyrusleeping
Copy link
Contributor

While we're on this topic. Should we talk about just not dialing peers in private subnets that we arent also in?

I'm fine merging this change as is, but maybe the next thing to do would be that?

@magik6k
Copy link
Contributor Author

magik6k commented Jun 12, 2018

While we're on this topic. Should we talk about just not dialing peers in private subnets that we arent also in?

I'd say that this should be done at the individual transport level, probably with some common helpers put to go-addr-util.

One problem is that in larger networks hosts tend to not know all lan routes. Simplest example would be [internets] <- NAT <- 10.0.0.0/8 net <- NAT <- 162.168.0.0/24 net <- ipfs instance. We'd now break connectivity to hosts in 10.0.0.0/8 network.

@whyrusleeping
Copy link
Contributor

One problem is that in larger networks hosts tend to not know all lan routes. Simplest example would be [internets] <- NAT <- 10.0.0.0/8 net <- NAT <- 162.168.0.0/24 net <- ipfs instance. We'd now break connectivity to hosts in 10.0.0.0/8 network.

Yeah... good point.

Copy link
Contributor

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Changes look good to me. Would like to give @bigs and @Stebalien (if he wants) a chance to review and 👍 too.

Copy link
Contributor

@bigs bigs left a comment

Choose a reason for hiding this comment

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

Looks great! Super clean & well orchestrated with the child Context

This was referenced May 25, 2022
@whyrusleeping
Copy link
Contributor

alright, let's ship this :)

@whyrusleeping whyrusleeping merged commit f144edf into master Jun 17, 2018
@ghost ghost removed the status/in-progress In progress label Jun 17, 2018
@whyrusleeping whyrusleeping deleted the feat/local-timeout branch June 17, 2018 15:37
@whyrusleeping
Copy link
Contributor

Thanks @magik6k!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants