Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

Fix goroutine leak by closing accounts manager on Node.Stop #1147

Closed
wants to merge 1 commit into from

Conversation

janos
Copy link
Member

@janos janos commented Jan 22, 2019

Fixes: #1142

While performing the test described in #1142 issue, a large number of gorutines were created and not terminated. Stack trace shows many goroutines:

goroutine 3370 [select]:
github.com/ethereum/go-ethereum/accounts.(*Manager).update(0xc0000b7520)
	/Users/janos/go/src/github.com/ethereum/go-ethereum/accounts/manager.go:95 +0x22a
created by github.com/ethereum/go-ethereum/accounts.NewManager
	/Users/janos/go/src/github.com/ethereum/go-ethereum/accounts/manager.go:68 +0x6f2

goroutine 4481 [select]:
github.com/ethereum/go-ethereum/accounts/keystore.(*watcher).loop(0xc0035ac9a0)
	/Users/janos/go/src/github.com/ethereum/go-ethereum/accounts/keystore/watch.go:94 +0x65b
created by github.com/ethereum/go-ethereum/accounts/keystore.(*watcher).start
	/Users/janos/go/src/github.com/ethereum/go-ethereum/accounts/keystore/watch.go:52 +0xb6

Accounts manager is created but not closed for node.Node, leaving large number of goroutines alive. This change closes accounts manager on Node.Stop, but Node initializes accounts manager in constructor, not no Start, so for Restart and Start to work as expected, accounts manager construction and closing is handled in Start as well, preserving the original required initialization in constructor.

Keystore watch creates fs watchers and we have noticed before a lot of (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-21) errors on macOS while running a large scale simulations. This change may improve simulations performance as it closes unneeded file watchers.

Note: Running tests described in #1142 time go test -race -count 100 github.com/ethereum/go-ethereum/swarm/network/simulation require changes from PR ethereum/go-ethereum#18464 to fix race conditions, but this PR solves goroutine leak. This tests run for about 20 minutes.

Copy link
Contributor

@frncmx frncmx left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@frncmx frncmx removed their assignment Jan 22, 2019
@frncmx
Copy link
Contributor

frncmx commented Jan 23, 2019

@janos I think in the upstream PR we should request a review from Felix and Peter, as I see they worked on this part mostly. That can take time, too.

So what do you think about submitting this upstream?

@janos
Copy link
Member Author

janos commented Jan 23, 2019

@frncmx yes, I'll submit upstream, to speed things up.

@janos
Copy link
Member Author

janos commented Jan 23, 2019

Submitted upstream ethereum/go-ethereum#18505.

@janos janos closed this Jan 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

network/simulation: Goroutine leak (simultaneously alive, limit exceeded)
2 participants