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

network: terminate Hive connect goroutine on Stop #1740

Merged
merged 1 commit into from
Sep 10, 2019
Merged

Conversation

janos
Copy link
Member

@janos janos commented Sep 10, 2019

This PR addresses the issue where Hive connect goroutine does not exit after calling Hive.Stop(), creating goroutone leaks. It appears that it was expected for Hive.ticker.C to be closed after Hive.ticker.Stop() is called, but that is not the case. Ticker channel is not closed leaving the for loop waiting indefinitely as no more ticks are sent.

This issue can be reproduced with this network ad hoc test:

func TestConnectLeak(t *testing.T) {
	s, pp, err := newHiveTester(NewHiveParams(), 1, nil)
	if err != nil {
		t.Fatal(err)
	}
	err = pp.Start(s.Server)
	if err != nil {
		t.Fatal(err)
	}
	pp.Stop()

	stackSymbol := []byte("github.com/ethersphere/swarm/network.(*Hive).connect")
	for i := 0; i < 100; i++ {
		buf := make([]byte, 1<<16)
		runtime.Stack(buf, true)
		if !bytes.Contains(buf, stackSymbol) {
			return
		}
		time.Sleep(10 * time.Millisecond)
	}
	t.Error("connect goroutine did not exit")
}

@janos janos self-assigned this Sep 10, 2019
Copy link
Member

@zelig zelig left a comment

Choose a reason for hiding this comment

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

oops nice catch

@janos janos merged commit 43f2b87 into master Sep 10, 2019
@janos janos deleted the hive-connect-exit branch September 10, 2019 14:52
@skylenet skylenet added this to the 0.5.0 milestone Sep 17, 2019
chadsr added a commit to chadsr/swarm that referenced this pull request Sep 23, 2019
* 'master' of github.com:ethersphere/swarm:
  network: terminate Hive connect goroutine on Stop (ethersphere#1740)
  Incentives rpc test (ethersphere#1733)
  swarm, swap: pass chequebook address at start-up (ethersphere#1718)
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.

4 participants