rate limit concurrent peer dials #1802

whyrusleeping · 2015-10-06T06:33:19Z

This should prevent the crashes when adding large files and directories.

addresses #1654 and other similar issues.

License: MIT
Signed-off-by: Jeromy jeromyj@gmail.com

whyrusleeping · 2015-10-06T06:34:54Z

p2p/net/swarm/swarm_dial.go

@@ -44,6 +44,9 @@ var (
 // add loop back in Dial(.)
 const dialAttempts = 1

+// dial at most 12 peers concurrently
+const concurrentPeerDials = 12


I chose 12 here because 12*10 (conccurent dials per peer) * 2 (reuseport) is around 1/4 of the available file descriptors on linux, and is still less than the 256 limit on OSX. Although maybe its a little too close to the OSX limit. Maybe we should set it at runtime with an OS check.

we can probably bump the 10 factor to 5. i worry about putting it lower than that, as otherwise we wont have the chance to prefer local addresses. once we have good estimators, maybe.

and, 12 max concurrent dials is a bit rough. Maybe this should only be a limit for socket-consuming transports?

thats really hard to tell at this point. The lock doesnt specify whats going to be dialed, just who.

How about dropping the per peer concurrency to 6 and raising the number of peers to 15? That puts us at a limit of 180 fd's (at least until we fix epoll)

jbenet · 2015-10-06T06:51:54Z

LGTM, though maybe:

also bump down the 10 concurrent addr-dials per outgoing dial
make the ratelimit only rate limit "fd-per-conn" based conns

Also, i believe that if you fix the reuseport epoll + kqueue fd screwup, that halves the used fds

whyrusleeping · 2015-10-06T16:05:55Z

rate limiting fd-per-conn dials will be a lot of work, and we dont have any such transports yet. I think its best to push this until a little later.

jbenet · 2015-10-06T16:17:51Z

the point is that we don't need to rate limit utp and we do need to rate limit tcp

—
Sent from Mailbox

On Tue, Oct 6, 2015 at 10:05 AM, Jeromy Johnson notifications@github.com
wrote:

rate limiting fd-per-conn dials will be a lot of work, and we dont have any such transports yet. I think its best to push this until a little later.

Reply to this email directly or view it on GitHub:
#1802 (comment)

whyrusleeping · 2015-10-06T16:47:33Z

Yeah, but Lock(peer) gives me no indication of what transport i'll actually be able to connect to them on. If we really want that, we would have to put it in the actual dial code, and then just limit the total number of concurrent tcp dials. or something like that (which works too)

jbenet · 2015-10-06T16:59:05Z

Yep, that's why I meant by harder :)

jbenet · 2015-10-06T16:59:25Z

(Think we should do this-- the limit will hurt)

jbenet · 2015-10-06T17:00:08Z

Good side effect: utp addresses much more likey to be chosen

whyrusleeping · 2015-10-06T17:53:24Z

p2p/net/swarm/swarm_dial.go

-				default:
+				if conn.IsTcpMultiaddr(remoteAddrs[i]) {
+					// For addresses that require an FD to dial
+					select {


this is ugly because you cant really do conditional select cases...

Here, this fixes it all for you:

func (s *Swarm) addrDialRateLimit(a ma.Multiaddr) (chan<- struct{}) { if isFDCostlyTransport(a) { return s.fdRateLimit } // do not rate limit it at all return make(chan struct{}, 1) } // and in here: for _, i := range rand.Perm(len(remoteAddrs)) { select { case <-foundConn: // if one of them succeeded already break case <-worker.Closing(): // our context was cancelled break default: } workerAddr := remoteAddrs[i] // shadow variable to avoid race // we have to do the waiting concurrently because there are addrs // that SHOULD NOT be rate limited (utp), nor blocked by other // rate limited addrs (tcp). // we need to call limiter.Go as required by goproc/limiter semantics. // note: limiter.Go is not limiter.LimitedGo. limiter.Go(func(p process.Process) { // returns whatever ratelimiting is acceptable for workerAddr. // may not rate limit at all. rl := s.addrDialRateLimit(workerAddr) rl<- struct{}{} limiter.LimitedGo(func(worker process.Process) { dialSingleAddr(workerAddr) }) <-rl }) }

whyrusleeping · 2015-10-06T17:53:42Z

@jbenet alright, we do the thing now.

whyrusleeping · 2015-10-07T06:14:52Z

RFCR/M

jbenet · 2015-10-08T07:52:28Z

p2p/net/conn/dial.go

+func IsTcpMultiaddr(a ma.Multiaddr) bool {
+	p := a.Protocols()
+	return len(p) == 2 && (p[0].Name == "ip4" || p[0].Name == "ip6") && p[1].Name == "tcp"
+}


probably should have:

func isFDCostlyTransport(a ma.Multiaddr) bool { return isTCPMultiaddr(a) }

jbenet · 2015-10-08T08:20:04Z

This is very weird: https://gist.github.com/jbenet/d79f4b9cfcfeea48363e

License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>

jbenet · 2015-10-08T08:21:49Z

@whyrusleeping i pushed here -- fixed it for you i think 1c2223d

except for whatever is going on in #1802 (comment)

whyrusleeping · 2015-10-08T15:28:51Z

@jbenet what changed in that commit?

whyrusleeping · 2015-10-08T15:34:01Z

also, that gist is quite weird. But I doubt that "what weve been seeing isnt too many fd's", I've reproduced the error many times and confirmed the number of fd's on the system (yay for filesystem notifications on the /proc/pid/fd directories).

what did you repro that on? your laptop? does that happen every time?

whyrusleeping · 2015-10-08T17:46:39Z

@jbenet i cant repro the thing you posted. Was it a fluke? Or can you repro reliably?

whyrusleeping · 2015-10-08T18:00:47Z

made my roommate test this on OSX for me, looks like its an OSX issue. It also happens on master. TestSimultOpenMany opens more file descriptors than OSX can handle by default, we dont see the failures in CI because for CI we lower the number of rounds to run by half. That test opening too many file descriptors causes all the rest to fail because theyre all in the same process.

GitCop · 2015-10-08T18:05:12Z

There were the following issues with your Pull Request

Commit: f1f068d
- Invalid signoff. Commit message must end with
  License: MIT
  Signed-off-by: .* <.*>

Guidelines and a script are available to help. Your feedback on GitCop is welcome on this issue.

This message was auto-generated by https://gitcop.com

whyrusleeping · 2015-10-08T18:05:57Z

@GitCop killjoy.

License: MIT Signed-off-by: Jeromy Johnson <why@ipfs.io>

whyrusleeping · 2015-10-08T18:07:33Z

@jbenet that should fix your issue.

rate limit concurrent peer dials

jbenet added the status/in-progress label Oct 6, 2015

whyrusleeping reviewed Oct 6, 2015
View reviewed changes

whyrusleeping force-pushed the fix/too-many-fd branch from 82084fe to 2787ba0 Compare October 6, 2015 15:58

whyrusleeping force-pushed the fix/too-many-fd branch from 2787ba0 to 5e5967e Compare October 6, 2015 17:52

whyrusleeping reviewed Oct 6, 2015
View reviewed changes

whyrusleeping added this to the 0.3.8 milestone Oct 7, 2015

whyrusleeping mentioned this pull request Oct 7, 2015

Feat/utp #1789

Closed

jbenet reviewed Oct 8, 2015
View reviewed changes

rate limit concurrent peer dials

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

1c2223d

License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>

jbenet force-pushed the fix/too-many-fd branch from 5e5967e to 1c2223d Compare October 8, 2015 08:21

fix too many FD error on osx

c1c9a74

License: MIT Signed-off-by: Jeromy Johnson <why@ipfs.io>

whyrusleeping force-pushed the fix/too-many-fd branch from f1f068d to c1c9a74 Compare October 8, 2015 18:07

whyrusleeping assigned jbenet Oct 9, 2015

jbenet added a commit that referenced this pull request Oct 9, 2015

Merge pull request #1802 from ipfs/fix/too-many-fd

26d40b2

rate limit concurrent peer dials

jbenet merged commit 26d40b2 into master Oct 9, 2015

jbenet deleted the fix/too-many-fd branch October 9, 2015 20:06

jbenet removed the status/in-progress label Oct 9, 2015

whyrusleeping mentioned this pull request Oct 11, 2015

rate limiting dials #1800

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rate limit concurrent peer dials #1802

rate limit concurrent peer dials #1802

whyrusleeping commented Oct 6, 2015

whyrusleeping Oct 6, 2015

jbenet Oct 6, 2015

jbenet Oct 6, 2015

whyrusleeping Oct 6, 2015

jbenet commented Oct 6, 2015

whyrusleeping commented Oct 6, 2015

jbenet commented Oct 6, 2015

rate limiting fd-per-conn dials will be a lot of work, and we dont have any such transports yet. I think its best to push this until a little later.

whyrusleeping commented Oct 6, 2015

jbenet commented Oct 6, 2015

jbenet commented Oct 6, 2015

jbenet commented Oct 6, 2015

whyrusleeping Oct 6, 2015

jbenet Oct 8, 2015

whyrusleeping commented Oct 6, 2015

whyrusleeping commented Oct 7, 2015

jbenet Oct 8, 2015

jbenet commented Oct 8, 2015

jbenet commented Oct 8, 2015

whyrusleeping commented Oct 8, 2015

whyrusleeping commented Oct 8, 2015

whyrusleeping commented Oct 8, 2015

whyrusleeping commented Oct 8, 2015

GitCop commented Oct 8, 2015

whyrusleeping commented Oct 8, 2015

whyrusleeping commented Oct 8, 2015

rate limit concurrent peer dials #1802

rate limit concurrent peer dials #1802

Conversation

whyrusleeping commented Oct 6, 2015

whyrusleeping Oct 6, 2015

Choose a reason for hiding this comment

jbenet Oct 6, 2015

Choose a reason for hiding this comment

jbenet Oct 6, 2015

Choose a reason for hiding this comment

whyrusleeping Oct 6, 2015

Choose a reason for hiding this comment

jbenet commented Oct 6, 2015

whyrusleeping commented Oct 6, 2015

jbenet commented Oct 6, 2015

rate limiting fd-per-conn dials will be a lot of work, and we dont have any such transports yet. I think its best to push this until a little later.

whyrusleeping commented Oct 6, 2015

jbenet commented Oct 6, 2015

jbenet commented Oct 6, 2015

jbenet commented Oct 6, 2015

whyrusleeping Oct 6, 2015

Choose a reason for hiding this comment

jbenet Oct 8, 2015

Choose a reason for hiding this comment

whyrusleeping commented Oct 6, 2015

whyrusleeping commented Oct 7, 2015

jbenet Oct 8, 2015

Choose a reason for hiding this comment

jbenet commented Oct 8, 2015

jbenet commented Oct 8, 2015

whyrusleeping commented Oct 8, 2015

whyrusleeping commented Oct 8, 2015

whyrusleeping commented Oct 8, 2015

whyrusleeping commented Oct 8, 2015

GitCop commented Oct 8, 2015

whyrusleeping commented Oct 8, 2015

whyrusleeping commented Oct 8, 2015