Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle incoming conns in their own goroutines #35

Merged
merged 5 commits into from
Apr 11, 2016

Conversation

whyrusleeping
Copy link
Contributor

Doing the multistream negotiation in sync causes hanging issues.
This commit accepts transport connections and starts the negotiation
in a separate goroutine, sending it down a channel when its ready.

Doing the multistream negotiation in sync causes hanging issues.
This commit accepts transport connections and starts the negotiation
in a separate goroutine, sending it down a channel when its ready.
}

c.Close()
<-done
Copy link
Contributor

Choose a reason for hiding this comment

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

good basic test case. but we need a stronger stress tests. something like:

  1. start off N (=1000) goroutines, all hang for ~20ms, then continue. measure that they all complete async (within < 2s, instead of serially N * 20 = 20s)
  2. start off N (=1000) goroutines, hang half of them until timeout. the others should complete shortly after kicking off.
  3. start off N (=1000) goroutines, dial, hang for some (1-10)ms , complete them. check listener goroutines are all closed properly (i.e. total goroutines << N)

Copy link
Contributor

Choose a reason for hiding this comment

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

hanging to timeout is important. can decrease the tiemeout for test, or mock it out with a channel to verify things make forward progress correctly

if took > limit {
t.Fatal("took too long!")
}
log.Errorf("took: %s (less than %s)", took, limit)
Copy link
Contributor

Choose a reason for hiding this comment

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

need to call wg.Wait() before exiting this function.

@jbenet
Copy link
Contributor

jbenet commented Apr 11, 2016

Thanks for handling the changes. This is pretty close. couple things left above o/

"author": "whyrusleeping",
"name": "gorocheck",
"hash": "QmWBTBj2ceC8gk2pS5scr4WwuHGrzsTkUnPpv1KPhYET57",
"version": "0.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

would be great to have the "pkg repo" (individual pkg repo where you can report issues) listed in gx to get it from stored here. this could be a link, http for now and ipns name in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could do that, thats a good idea.

@jbenet
Copy link
Contributor

jbenet commented Apr 11, 2016

I tried running tests locally on go 1.6 and:

--- FAIL: TestConcurrentAccept (0.15s)
panic: had 1 goroutines still running. First on list: [os/signal.signal_recv(0x0)   /usr/local/go/src/runtime/sigqueue.go:116 +0x132 os/signal.loop()   /usr/local/go/src/os/signal/signal_unix.go:22 +0x18 created by os/signal.init.1     /usr/local/go/src/os/signal/signal_unix.go:28 +0x37] [recovered]
    panic: had 1 goroutines still running. First on list: [os/signal.signal_recv(0x0)   /usr/local/go/src/runtime/sigqueue.go:116 +0x132 os/signal.loop()   /usr/local/go/src/os/signal/signal_unix.go:22 +0x18 created by os/signal.init.1     /usr/local/go/src/os/signal/signal_unix.go:28 +0x37]

goroutine 80 [running]:
panic(0x44b200, 0xc8203832e0)
    /usr/local/go/src/runtime/panic.go:464 +0x3e6
testing.tRunner.func1(0xc820388240)
    /usr/local/go/src/testing/testing.go:467 +0x192
panic(0x44b200, 0xc8203832e0)
    /usr/local/go/src/runtime/panic.go:426 +0x4e9
github.com/ipfs/go-libp2p/p2p/net/conn.TestConcurrentAccept(0xc820388240)
    /Users/jbenet/go/src/github.com/ipfs/go-libp2p/p2p/net/conn/dial_test.go:530 +0x9a5
testing.tRunner(0xc820388240, 0x75b5a0)
    /usr/local/go/src/testing/testing.go:473 +0x98
created by testing.RunTests
    /usr/local/go/src/testing/testing.go:582 +0x892
exit status 2
FAIL    github.com/ipfs/go-libp2p/p2p/net/conn  0.686s

@jbenet
Copy link
Contributor

jbenet commented Apr 11, 2016

passes fine for me in 1.5.3. Looks like gorocheck doesn't account something new in 1.6?

@whyrusleeping
Copy link
Contributor Author

alright, this LGTM. gonna ship and get it into go-ipfs

@jbenet
Copy link
Contributor

jbenet commented Apr 11, 2016

> go-use 1.6
using: go version go1.6 darwin/amd64
> go test
13:41:58.883 ERROR       conn: took: 34.695497ms (less than 2s) dial_test.go:521
skipped  &{gorocheck.parseStack 130  [gx/ipfs/QmWdkzYigQqvWn7DhthjhEmJ4gVQc3R7VNrjXTwMmmLjMf/gorocheck.parseStack(0x0, 0x0, 0x0)    /Users/jbenet/go/src/gx/ipfs/QmWdkzYigQqvWn7DhthjhEmJ4gVQc3R7VNrjXTwMmmLjMf/gorocheck/gorochecker.go:37 +0xa8 gx/ipfs/QmWdkzYigQqvWn7DhthjhEmJ4gVQc3R7VNrjXTwMmmLjMf/gorocheck.CheckForLeaks(0x5cdf90, 0x0, 0x0)    /Users/jbenet/go/src/gx/ipfs/QmWdkzYigQqvWn7DhthjhEmJ4gVQc3R7VNrjXTwMmmLjMf/gorocheck/gorochecker.go:91 +0x36 github.com/ipfs/go-libp2p/p2p/net/conn.TestConcurrentAccept(0xc82016c090)     /Users/jbenet/go/src/github.com/ipfs/go-libp2p/p2p/net/conn/dial_test.go:528 +0x95d testing.tRunner(0xc82016c090, 0x75c5a0)     /usr/local/go/src/testing/testing.go:473 +0x98 created by testing.RunTests  /usr/local/go/src/testing/testing.go:582 +0x892 ]}
skipped  &{testing.RunTests 1  [testing.RunTests(0x5ce4c0, 0x75c4e0, 0xf, 0xf, 0x4fff01)    /usr/local/go/src/testing/testing.go:583 +0x8d2 testing.(*M).Run(0xc820476f08, 0x4db160)    /usr/local/go/src/testing/testing.go:515 +0x81 main.main()  github.com/ipfs/go-libp2p/p2p/net/conn/_test/_testmain.go:82 +0x117 ]}
skipped  &{runtime.goexit 17  [runtime.goexit()     /usr/local/go/src/runtime/asm_amd64.s:1998 +0x1 ]}
skipped  &{signal.signal_recv 6  [os/signal.signal_recv(0x0)    /usr/local/go/src/runtime/sigqueue.go:116 +0x132 os/signal.loop()   /usr/local/go/src/os/signal/signal_unix.go:22 +0x18 created by os/signal.init.1     /usr/local/go/src/os/signal/signal_unix.go:28 +0x37]}
skipped  &{gorocheck.parseStack 338  [gx/ipfs/QmWdkzYigQqvWn7DhthjhEmJ4gVQc3R7VNrjXTwMmmLjMf/gorocheck.parseStack(0x0, 0x0, 0x0)    /Users/jbenet/go/src/gx/ipfs/QmWdkzYigQqvWn7DhthjhEmJ4gVQc3R7VNrjXTwMmmLjMf/gorocheck/gorochecker.go:37 +0xa8 gx/ipfs/QmWdkzYigQqvWn7DhthjhEmJ4gVQc3R7VNrjXTwMmmLjMf/gorocheck.CheckForLeaks(0x5cdf90, 0x0, 0x0)    /Users/jbenet/go/src/gx/ipfs/QmWdkzYigQqvWn7DhthjhEmJ4gVQc3R7VNrjXTwMmmLjMf/gorocheck/gorochecker.go:91 +0x36 github.com/ipfs/go-libp2p/p2p/net/conn.TestConnectionTimeouts(0xc82016c120)   /Users/jbenet/go/src/github.com/ipfs/go-libp2p/p2p/net/conn/dial_test.go:645 +0xa2d testing.tRunner(0xc82016c120, 0x75c5b8)     /usr/local/go/src/testing/testing.go:473 +0x98 created by testing.RunTests  /usr/local/go/src/testing/testing.go:582 +0x892 ]}
skipped  &{testing.RunTests 1  [testing.RunTests(0x5ce4c0, 0x75c4e0, 0xf, 0xf, 0x4fff01)    /usr/local/go/src/testing/testing.go:583 +0x8d2 testing.(*M).Run(0xc820256f08, 0x4db160)    /usr/local/go/src/testing/testing.go:515 +0x81 main.main()  github.com/ipfs/go-libp2p/p2p/net/conn/_test/_testmain.go:82 +0x117 ]}
skipped  &{runtime.goexit 17  [runtime.goexit()     /usr/local/go/src/runtime/asm_amd64.s:1998 +0x1 ]}
skipped  &{signal.signal_recv 6  [os/signal.signal_recv(0x0)    /usr/local/go/src/runtime/sigqueue.go:116 +0x132 os/signal.loop()   /usr/local/go/src/os/signal/signal_unix.go:22 +0x18 created by os/signal.init.1     /usr/local/go/src/os/signal/signal_unix.go:28 +0x37]}
PASS
ok      github.com/ipfs/go-libp2p/p2p/net/conn  6.017s

@whyrusleeping whyrusleeping merged commit b12b73d into master Apr 11, 2016
@whyrusleeping whyrusleeping deleted the fix/listener-hang branch April 11, 2016 18:16
marten-seemann pushed a commit that referenced this pull request Dec 20, 2021
* Single goroutine managing autonat-relevent events.
* Watching incoming connections and local interface changes as signals.
* Emit a single 'rechabilitychanged' persistent event

fix #40 
fix #36 
fix #35
fix #34 
fix #11
obsolete #28
obsolete #9 

Co-authored-by: Aarsh Shah <aarshkshah1992@gmail.com>
Co-authored-by: Adin Schmahmann <adin.schmahmann@gmail.com>
marten-seemann pushed a commit that referenced this pull request Apr 21, 2022
marten-seemann added a commit that referenced this pull request Apr 26, 2022
marten-seemann added a commit that referenced this pull request May 18, 2022
fix deadlock in the transport's serve function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants