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

bmt: fix data races in Hasher #2100

Merged
merged 1 commit into from
Feb 12, 2020
Merged

bmt: fix data races in Hasher #2100

merged 1 commit into from
Feb 12, 2020

Conversation

janos
Copy link
Member

@janos janos commented Feb 11, 2020

This PR fixes two data races found in bmt hasher.

Fixes are minimal, but maybe locking can be improved a bit more.

==================
WARNING: DATA RACE
Write at 0x00c000080418 by goroutine 87:
  github.com/ethersphere/swarm/bmt.(*Hasher).WriteSection()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt.go:445 +0x71

Previous read at 0x00c000080418 by goroutine 39:
  github.com/ethersphere/swarm/bmt.(*Hasher).Sum()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt.go:338 +0xae
  github.com/ethersphere/swarm/bmt.syncHash()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt_test.go:454 +0xd9
  github.com/ethersphere/swarm/bmt.testHasherCorrectness()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt_test.go:318 +0x32b
  github.com/ethersphere/swarm/bmt.TestSyncHasherCorrectness.func1()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt_test.go:163 +0x2ed
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:909 +0x199

Goroutine 87 (running) created at:
  github.com/ethersphere/swarm/bmt.(*Hasher).Write()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt.go:398 +0x3f9
  github.com/ethersphere/swarm/bmt.syncHash()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt_test.go:453 +0xba
  github.com/ethersphere/swarm/bmt.testHasherCorrectness()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt_test.go:318 +0x32b
  github.com/ethersphere/swarm/bmt.TestSyncHasherCorrectness.func1()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt_test.go:163 +0x2ed
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:909 +0x199

Goroutine 39 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:960 +0x651
  github.com/ethersphere/swarm/bmt.TestSyncHasherCorrectness()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt_test.go:154 +0x163
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:909 +0x199
==================
==================
WARNING: DATA RACE
Read at 0x00c0000868d8 by goroutine 170:
  github.com/ethersphere/swarm/bmt.(*Hasher).WriteSection()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt.go:445 +0x55

Previous write at 0x00c0000868d8 by goroutine 166:
  github.com/ethersphere/swarm/bmt.(*Hasher).Write()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt.go:367 +0x9e
  github.com/ethersphere/swarm/bmt.TestBMTWriterBuffers.func1.1()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt_test.go:262 +0x23a
  github.com/ethersphere/swarm/bmt.TestBMTWriterBuffers.func1.2()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt_test.go:280 +0x34

Goroutine 170 (running) created at:
  github.com/ethersphere/swarm/bmt.(*Hasher).Write()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt.go:398 +0x3f9
  github.com/ethersphere/swarm/bmt.TestBMTWriterBuffers.func1.1()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt_test.go:262 +0x23a
  github.com/ethersphere/swarm/bmt.TestBMTWriterBuffers.func1.2()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt_test.go:280 +0x34

Goroutine 166 (running) created at:
  github.com/ethersphere/swarm/bmt.TestBMTWriterBuffers.func1()
      /Users/janos/go/src/github.com/ethersphere/swarm/bmt/bmt_test.go:279 +0x5ca
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:909 +0x199
==================

@janos janos requested review from zelig and nolash February 11, 2020 14:39
@janos janos self-assigned this Feb 11, 2020
@janos janos added this to Backlog in Swarm Core - Sprint planning via automation Feb 11, 2020
@nolash
Copy link
Contributor

nolash commented Feb 11, 2020

what effect does it have on performance?

@janos
Copy link
Member Author

janos commented Feb 11, 2020

I do not know, but it has effect on correctness.

Copy link
Contributor

@nolash nolash left a comment

Choose a reason for hiding this comment

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

Thanks for finding this. Let's revisit the optimizations later. Anyway this doesn't affect performance much.

@acud acud added this to the 0.5.7 milestone Feb 12, 2020
@acud acud merged commit c79587c into master Feb 12, 2020
Swarm Core - Sprint planning automation moved this from Backlog to Done Feb 12, 2020
@acud acud deleted the bmt-data-races branch February 12, 2020 03:04
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