-
Notifications
You must be signed in to change notification settings - Fork 20.6k
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
node, p2p/simulations: fix node.Node AccountsManager leak #19004
Conversation
// Close releases resources acquired in Node constructor New. | ||
func (n *Node) Close() error { | ||
return n.accman.Close() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also add a shutdown attempt to make sure everything is cleaned up if someone only calls Close, but not Stop.
func (n *Node) Close() error {
if err := n.Stop(); err != nil && err != ErrNodeStopped {
return err
}
return n.accman.Close()
}
If you want to be truly correct, you could close both first, and only then return the errors, possible returning a combined error if both failed.
func (n *Node) Close() error {
var errs []error
// Terminate all subsystems and collect any errors
if err := n.Stop(); err != nil && err != ErrNodeStopped {
errs = append(errs, err)
}
if err := n.accman.Close(); err != nil {
errs = append(errs, err)
}
// Report any errors that might have occurred
switch len(errs) {
case 0: return nil
case 1: return errs[0]
default: return fmt.Errorf("%v", errs)
}
}
We also have quite a few |
Thanks @karalabe for review. I've updated this PR based on your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah wait, could you please go over the tests and add the defer Close-es in the node package too?
Yes, Nodes in node tests are now closed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (I made a few minor code tweaks)
Thanks, @karalabe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
* node: close AccountsManager in new Close method * p2p/simulations, p2p/simulations/adapters: handle node close on shutdown * node: move node ephemeralKeystore cleanup to stop method * node: call Stop in Node.Close method * cmd/geth: close node.Node created with makeFullNode in cli commands * node: close Node instances in tests * cmd/geth, node: minor code style fixes * cmd, console, miner, mobile: proper node Close() termination
This PR is a second take on fixing the AccountsManager leak on node.Node. The first PR was rejected #18505.
Fixes: ethersphere/swarm#1142.
Swarm PR: ethersphere/swarm#1194.
While performing the test described in ethersphere/swarm#1142 issue, a large number of gorutines were created and not terminated. Stack trace shows many goroutines:
Accounts manager is created but not closed for node.Node, leaving large number of goroutines alive. This change closes accounts manager on new method Node.Close, moves the cleaning of ephemeral directory from Stop to Close, and handles the closing of the node in p2p/simulations.Network through p2p/simulations/adapters.SimNode.