Skip to content

Conversation

@Chain-Fox
Copy link
Contributor

subscribe() in event/subscribe.go and Stop() in p2p/simulations/adapters/exec.go share a same problem: some goroutines in these functions will be leaked in certain cases (unsub for subscribe(), and timeout for Stop()).
In subscribe(), the send operation subscribed <- err on L115 will be blocked forever when unsub is selected. Thus the child goroutine leaks and always occupies the memory.
Adding one buffer to the channel will make the send operation never block the child goroutine,
and it will not change semantic.
The same is true for Stop().
In Stop(), although Kill() is called on timeout,
I am not sure if the child goroutine will be in the same process or not. Here is the annotation of Kill(), in /os/exec.go:

// Kill causes the Process to exit immediately. Kill does not wait until
// the Process has actually exited. This only kills the Process itself,
// not any other processes it may have started.
func (p *Process) Kill() error {
    return p.kill()
}

Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

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

Very nice finding!


func (s *resubscribeSub) subscribe() Subscription {
subscribed := make(chan error)
subscribed := make(chan error, 1)
Copy link
Contributor

@fjl fjl Feb 12, 2020

Choose a reason for hiding this comment

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

I think it would be better to fix this case by waiting on the subscribed channel when the subscribe loop exits. This way, Unsubscribe will block until the goroutine running s.fn has exited.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I have now applied the fix myself.

return n.Cmd.Process.Kill()
}
waitErr := make(chan error)
waitErr := make(chan error, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

This fix is OK as-is.

@fjl fjl added this to the 1.9.11 milestone Feb 12, 2020
@fjl fjl merged commit a9614c3 into ethereum:master Feb 12, 2020
enriquefynn pushed a commit to enriquefynn/go-ethereum that referenced this pull request Mar 10, 2021
gzliudan pushed a commit to gzliudan/XDPoSChain that referenced this pull request Sep 19, 2025
gzliudan pushed a commit to gzliudan/XDPoSChain that referenced this pull request Sep 19, 2025
gzliudan added a commit to XinFinOrg/XDPoSChain that referenced this pull request Sep 21, 2025
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