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

Fix pss data races #1900

Merged
merged 2 commits into from
Oct 28, 2019
Merged

Fix pss data races #1900

merged 2 commits into from
Oct 28, 2019

Conversation

janos
Copy link
Member

@janos janos commented Oct 23, 2019

This PR fixes two data races found in PSS. Both are related to production code, not only test code.

One is related to Ticker.quitC being set on Stop while waiting on the same channel in a different goroutine which can cause reading from the nil channel which blocks forever resulting in infinite ticker and goroutine leak.

==================
WARNING: DATA RACE
Write at 0x00c0001ec098 by goroutine 28:
  github.com/ethersphere/swarm/pss.(*Pss).Stop()
      /Users/janos/go/src/github.com/ethersphere/swarm/pss/internal/ticker/ticker.go:52 +0xbc
  github.com/ethersphere/swarm/pss.TestForwardBasic()
      /Users/janos/go/src/github.com/ethersphere/swarm/pss/forwarding_test.go:236 +0x2d17
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:909 +0x199

Previous read at 0x00c0001ec098 by goroutine 29:
  github.com/ethersphere/swarm/pss/internal/ticker.New.func1()
      /Users/janos/go/src/github.com/ethersphere/swarm/pss/internal/ticker/ticker.go:38 +0xd6

Goroutine 28 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:960 +0x651
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:1202 +0xa6
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:909 +0x199
  testing.runTests()
      /usr/local/go/src/testing/testing.go:1200 +0x521
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:1117 +0x2ff
  main.main()
      _testmain.go:92 +0x223

Goroutine 29 (running) created at:
  github.com/ethersphere/swarm/pss/internal/ticker.New()
      /Users/janos/go/src/github.com/ethersphere/swarm/pss/internal/ticker/ticker.go:32 +0x10f
  github.com/ethersphere/swarm/pss.New()
      /Users/janos/go/src/github.com/ethersphere/swarm/pss/pss.go:184 +0x8f7
  github.com/ethersphere/swarm/pss.createPss()
      /Users/janos/go/src/github.com/ethersphere/swarm/pss/forwarding_test.go:329 +0x12d
  github.com/ethersphere/swarm/pss.TestForwardBasic()
      /Users/janos/go/src/github.com/ethersphere/swarm/pss/forwarding_test.go:55 +0x7c0
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:909 +0x199
==================
--- FAIL: TestForwardBasic (0.01s)
    testing.go:853: race detected during execution of test

Another data race is on mutating and reading capabilities from topicHandlerCaps which can result in incorrect raw or prox states in handlePssMsg. The original logic in PSS.Register should be preserved.

==================
WARNING: DATA RACE
Write at 0x00c00583efc0 by goroutine 1254:
  github.com/ethersphere/swarm/pss.(*Pss).Register()
      /Users/janos/go/src/github.com/ethersphere/swarm/pss/pss.go:364 +0x43b
  github.com/ethersphere/swarm/pss.TestRawAllow()
      /Users/janos/go/src/github.com/ethersphere/swarm/pss/pss_test.go:584 +0xa6b
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:909 +0x199

Previous read at 0x00c00583efc0 by goroutine 1314:
  github.com/ethersphere/swarm/pss.(*Pss).handlePssMsg()
      /Users/janos/go/src/github.com/ethersphere/swarm/pss/pss.go:436 +0xc1b
  github.com/ethersphere/swarm/pss.(*Pss).handle.func1()
      /Users/janos/go/src/github.com/ethersphere/swarm/pss/pss.go:403 +0x95

Goroutine 1254 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:960 +0x651
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:1202 +0xa6
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:909 +0x199
  testing.runTests()
      /usr/local/go/src/testing/testing.go:1200 +0x521
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:1117 +0x2ff
  main.main()
      _testmain.go:92 +0x223

Goroutine 1314 (finished) created at:
  github.com/ethersphere/swarm/pss.(*Pss).handle()
      /Users/janos/go/src/github.com/ethersphere/swarm/pss/pss.go:397 +0x82
  github.com/ethersphere/swarm/pss.TestRawAllow()
      /Users/janos/go/src/github.com/ethersphere/swarm/pss/pss_test.go:570 +0x88d
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:909 +0x199
==================

The acceptance criteria is that go test -count 1 ./pss -race passes successfully.

@janos janos self-assigned this Oct 23, 2019
@janos janos added this to Backlog in Swarm Core - Sprint planning via automation Oct 23, 2019
@acud acud moved this from Backlog to In review (includes Documentation) in Swarm Core - Sprint planning Oct 23, 2019
@janos janos mentioned this pull request Oct 25, 2019
@janos janos merged commit fa6a3f7 into master Oct 28, 2019
Swarm Core - Sprint planning automation moved this from In review (includes Documentation) to Done Oct 28, 2019
@janos janos deleted the fix-pss-data-races branch October 28, 2019 08:46
@acud acud added this to the 0.5.3 milestone Nov 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants