lni / dragonboat

A feature complete and high performance multi-group Raft library in Go.
Apache License 2.0
4.99k stars 534 forks source link

Don't start ITransport before Transport is fully initialized. #241

Closed vstarodub closed 2 years ago

vstarodub commented 2 years ago

This fixes a race condition, where t.handleRequest callback uses fields of Transport that were not yet initialised.

Race condition was originally found by race detector in v3.3.1:

WARNING: DATA RACE
Read at 0x00c0003a0aa8 by time="2022-04-21T11:13:11.11755167" level=info msg="nodehost address: 127.0.0.1:60004" component=dragonboat pkg=dragonboat
goroutine 223:
  github.com/lni/dragonboat/v3/internal/transport.(*Transport).handleRequest()
      /root/go/pkg/mod/github.com/lni/dragonboat/v3@v3.3.1/internal/transport/transport.go:330 +0x43b
  github.com/lni/dragonboat/v3/internal/transport.(*Transport).handleRequest-fm()
      /root/go/pkg/mod/github.com/lni/dragonboat/v3@v3.3.1/internal/transport/transport.go:308 +0xe5
  github.com/lni/dragonboat/v3/plugin/chan.(*ChanTransport).process()
      /root/go/pkg/mod/github.com/lni/dragonboat/v3@v3.3.1/plugin/chan/chan.go:235 +0x2c2
  github.com/lni/dragonboat/v3/plugin/chan.(*ChanTransport).serveConn()
      /root/go/pkg/mod/github.com/lni/dragonboat/v3@v3.3.1/plugin/chan/chan.go:246 +0x256
  github.com/lni/dragonboat/v3/plugin/chan.(*ChanTransport).Start.func2.2()
      /root/go/pkg/mod/github.com/lni/dragonboat/v3@v3.3.1/plugin/chan/chan.go:166 +0x67
  github.com/lni/goutils/syncutil.(*Stopper).runWorker.func1()
      /root/go/pkg/mod/github.com/lni/goutils@v1.3.0/syncutil/stopper.go:79 +0x12e
Previous write at 0x00c0003a0aa8 by goroutine 47:
  github.com/lni/dragonboat/v3/internal/transport.NewTransport()
      /root/go/pkg/mod/github.com/lni/dragonboat/v3@v3.3.1/internal/transport/transport.go:260 +0x11e8
  github.com/lni/dragonboat/v3.(*NodeHost).createTransport()
      /root/go/pkg/mod/github.com/lni/dragonboat/v3@v3.3.1/nodehost.go:1797 +0x28b
  github.com/lni/dragonboat/v3.NewNodeHost()
      /root/go/pkg/mod/github.com/lni/dragonboat/v3@v3.3.1/nodehost.go:370 +0x14a8
  mds/internal/raft/dragonboat.NewRaftMDS()
      /builds/sds/mds/internal/raft/dragonboat/mds_raft.go:101 +0x6b7
  mds/internal/raft/dragonboat.(*MDSRaftTestSuite).TestJoin()
      /builds/sds/mds/internal/raft/dragonboat/mds_raft_test.go:247 +0x2c8
  runtime.call16()
      /usr/local/go/src/runtime/asm_amd64.s:625 +0x48
  reflect.Value.Call()
      /usr/local/go/src/reflect/value.go:339 +0xd7
  github.com/stretchr/testify/suite.Run.func1()
      /root/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:158 +0x71c
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run路dwrap路21()
      /usr/local/go/src/testing/testing.go:1306 +0x47
Goroutine 223 (running) created at:
  github.com/lni/goutils/syncutil.(*Stopper).runWorker()
      /root/go/pkg/mod/github.com/lni/goutils@v1.3.0/syncutil/stopper.go:74 +0x198
  github.com/lni/goutils/syncutil.(*Stopper).RunWorker()
      /root/go/pkg/mod/github.com/lni/goutils@v1.3.0/syncutil/stopper.go:68 +0xd9
  github.com/lni/dragonboat/v3/plugin/chan.(*ChanTransport).Start.func2()
      /root/go/pkg/mod/github.com/lni/dragonboat/v3@v3.3.1/plugin/chan/chan.go:165 +0x7a
  github.com/lni/goutils/syncutil.(*Stopper).runWorker.func1()
      /root/go/pkg/mod/github.com/lni/goutils@v1.3.0/syncutil/stopper.go:79 +0x12e
Goroutine 47 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1306 +0x726
  github.com/stretchr/testify/suite.runTests()
      /root/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:203 +0x18f
  github.com/stretchr/testify/suite.Run()
      /root/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:176 +0x994
  mds/internal/raft/dragonboat.TestMDSRaft()
      /builds/sds/mds/internal/raft/dragonboat/mds_raft_test.go:319 +0xf8
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run路dwrap路21()
      /usr/local/go/src/testing/testing.go:1306 +0x47
lni commented 2 years ago

Thanks for the PR.

codecov-commenter commented 2 years ago

Codecov Report

Merging #241 (93f21a6) into master (68619d0) will increase coverage by 0.15%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #241      +/-   ##
==========================================
+ Coverage   84.12%   84.27%   +0.15%     
==========================================
  Files          10       10              
  Lines        4130     4130              
==========================================
+ Hits         3474     3480       +6     
+ Misses        405      402       -3     
+ Partials      251      248       -3     
Impacted Files Coverage Δ
engine.go 83.58% <0.00%> (-0.22%) :arrow_down:
node.go 80.51% <0.00%> (+0.31%) :arrow_up:
request.go 93.39% <0.00%> (+0.85%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 68619d0...93f21a6. Read the comment docs.