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

Shutdown notifications go routines #109

Merged
merged 2 commits into from
Oct 26, 2020

Conversation

hannahhoward
Copy link
Collaborator

Goals

Resolve go routine leak for notifications publishers

Implementation

  • write a benchmark that simulates the conditions where we are seeing this go routine leak
  • add a Startup() method to the publisher and don't start go routine automatically (not essential but generally we try not to start go-routines in constructors
  • call Shutdown() on the publisher whenever the module using it shuts down (MessageQueue & PeerResponseSender)

Benchmark results before:

go test -v -bench=BenchmarkRoundtripSuccess/test-repeated-disconnects-20-10000$ --memprofile=profile.out --cpuprofile=cpu.out
goos: darwin
goarch: amd64
pkg: github.com/ipfs/go-graphsync/benchmarks
BenchmarkRoundtripSuccess
BenchmarkRoundtripSuccess/test-repeated-disconnects-20-10000
    benchmark_test.go:130: Number of running go-routines: 866
    benchmark_test.go:130: Number of running go-routines: 2447
    benchmark_test.go:130: Number of running go-routines: 9548
    benchmark_test.go:130: Number of running go-routines: 25369
    benchmark_test.go:130: Number of running go-routines: 44630
BenchmarkRoundtripSuccess/test-repeated-disconnects-20-10000-16         	     231	   5326464 ns/op	 4678319 B/op	   69275 allocs/op
PASS
ok  	github.com/ipfs/go-graphsync/benchmarks	5.007s

Benchmark results afterward (wonder what is causing the remaining go routines to still be running... though also this may be coming from some of the dependencies and not graphsync itself):

go test -v -bench=BenchmarkRoundtripSuccess/test-repeated-disconnects-20-10000$ --memprofile=profile.out --cpuprofile=cpu.out
goos: darwin
goarch: amd64
pkg: github.com/ipfs/go-graphsync/benchmarks
BenchmarkRoundtripSuccess
BenchmarkRoundtripSuccess/test-repeated-disconnects-20-10000
    benchmark_test.go:130: Number of running go-routines: 26
    benchmark_test.go:130: Number of running go-routines: 47
    benchmark_test.go:130: Number of running go-routines: 68
    benchmark_test.go:130: Number of running go-routines: 89
BenchmarkRoundtripSuccess/test-repeated-disconnects-20-10000-16         	     213	   4750892 ns/op	 4646892 B/op	   69192 allocs/op
PASS
ok  	github.com/ipfs/go-graphsync/benchmarks	2.819s

make sure all notification publishers are actually shut down, and don't start them automatically
}
cancel()
time.Sleep(100 * time.Millisecond)
b.Logf("Number of running go-routines: %d", runtime.NumGoroutine())
Copy link
Member

Choose a reason for hiding this comment

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

I'd consider tracking using https://github.com/uber-go/goleak. If we use that liberally, we should be able to detect all leaks.

@hannahhoward hannahhoward merged commit ea899f5 into master Oct 26, 2020
@aschmahmann aschmahmann mentioned this pull request Feb 18, 2021
73 tasks
@mvdan mvdan deleted the fix/go-routines-notifications branch December 15, 2021 14:16
marten-seemann pushed a commit that referenced this pull request Mar 2, 2023
…esponses (#109)

* feat(deps): update to graphsync v0.4.1

* feat(graphsync): consume response channel

consume response channel so graphsync does not buffer responses in memory

* fix(deps): update graphsync to fix bug
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