fix data race in unit test #4918
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yacovm
reviewed
Jul 4, 2024
| @@ -564,7 +565,9 @@ func createBFTChainUsingMocks(t *testing.T, node *Node, configInfo *ConfigInfo) | |||
| return | |||
| } | |||
| t.Logf("Node %d requested SendConsensus to node %d of type <%s>", node.NodeId, targetNodeId, reflect.TypeOf(message.GetContent())) | |||
| l.Lock() | |||
There was a problem hiding this comment.
why fix it this way? You can just define it as a local variable instead.
There was a problem hiding this comment.
err := node.sendMessage(node.NodeId, targetNodeId, message)
There was a problem hiding this comment.
Sorry, didn't quite understand your comment.
There was a problem hiding this comment.
where is the data race? is it in the write to the error variable? Or is it somewhere else?
There was a problem hiding this comment.
Two goroutines are trying to access the same resource. If you create a mutex inside the function func(targetNodeId uint64, message *smartbftprotos.Message), then I think that the mutex will be created each time the function is called and they will be different.
There was a problem hiding this comment.
where is the data race? is it in the write to the error variable? Or is it somewhere else?
maybe, I'll check it out.
There was a problem hiding this comment.
changed the code, now I'll wait for the tests.
There was a problem hiding this comment.
where is the data race? is it in the write to the error variable? Or is it somewhere else?
maybe, I'll check it out.
can you perhaps paste the data race trace so it will be more clear where it is?
There was a problem hiding this comment.
2024-07-02T12:41:09.1287048Z ==================
2024-07-02T12:41:09.1287157Z WARNING: DATA RACE
2024-07-02T12:41:09.1287296Z Write at 0x00c00197c750 by goroutine 1298:
2024-07-02T12:41:09.1287762Z github.com/hyperledger/fabric/orderer/consensus/smartbft_test.createBFTChainUsingMocks.func14()
2024-07-02T12:41:09.1288554Z /home/runner/work/fabric/fabric/orderer/consensus/smartbft/util_network_test.go:567 +0x314
2024-07-02T12:41:09.1289296Z github.com/hyperledger/fabric/orderer/consensus/smartbft_test.createBFTChainUsingMocks.(*EgressComm_SendConsensus_Call).Run.func21()
2024-07-02T12:41:09.1289815Z /home/runner/work/fabric/fabric/orderer/consensus/smartbft/mocks/egress_comm.go:90 +0xc3
2024-07-02T12:41:09.1290041Z github.com/stretchr/testify/mock.(*Mock).MethodCalled()
2024-07-02T12:41:09.1290539Z /home/runner/work/fabric/fabric/vendor/github.com/stretchr/testify/mock/mock.go:554 +0x129a
2024-07-02T12:41:09.1290724Z github.com/stretchr/testify/mock.(*Mock).Called()
2024-07-02T12:41:09.1291222Z /home/runner/work/fabric/fabric/vendor/github.com/stretchr/testify/mock/mock.go:466 +0x195
2024-07-02T12:41:09.1291652Z github.com/hyperledger/fabric/orderer/consensus/smartbft/mocks.(*EgressComm).SendConsensus()
2024-07-02T12:41:09.1292163Z /home/runner/work/fabric/fabric/orderer/consensus/smartbft/mocks/egress_comm.go:73 +0xe5
2024-07-02T12:41:09.1292712Z github.com/hyperledger-labs/SmartBFT/internal/bft.(*Controller).BroadcastConsensus()
2024-07-02T12:41:09.1293454Z /home/runner/work/fabric/fabric/vendor/github.com/hyperledger-labs/SmartBFT/internal/bft/controller.go:918 +0xcd
2024-07-02T12:41:09.1293842Z github.com/hyperledger-labs/SmartBFT/internal/bft.(*Controller).fetchState()
2024-07-02T12:41:09.1294577Z /home/runner/work/fabric/fabric/vendor/github.com/hyperledger-labs/SmartBFT/internal/bft/controller.go:714 +0x1f3
2024-07-02T12:41:09.1294926Z github.com/hyperledger-labs/SmartBFT/internal/bft.(*Controller).sync()
2024-07-02T12:41:09.1295657Z /home/runner/work/fabric/fabric/vendor/github.com/hyperledger-labs/SmartBFT/internal/bft/controller.go:640 +0xaa7
2024-07-02T12:41:09.1296086Z github.com/hyperledger-labs/SmartBFT/internal/bft.(*Controller).run()
2024-07-02T12:41:09.1296817Z /home/runner/work/fabric/fabric/vendor/github.com/hyperledger-labs/SmartBFT/internal/bft/controller.go:512 +0x471
2024-07-02T12:41:09.1297217Z github.com/hyperledger-labs/SmartBFT/internal/bft.(*Controller).Start.func1()
2024-07-02T12:41:09.1297933Z /home/runner/work/fabric/fabric/vendor/github.com/hyperledger-labs/SmartBFT/internal/bft/controller.go:810 +0x84
2024-07-02T12:41:09.1297943Z
2024-07-02T12:41:09.1298128Z Previous write at 0x00c00197c750 by goroutine 1295:
2024-07-02T12:41:09.1298592Z github.com/hyperledger/fabric/orderer/consensus/smartbft_test.createBFTChainUsingMocks.func14()
2024-07-02T12:41:09.1299100Z /home/runner/work/fabric/fabric/orderer/consensus/smartbft/util_network_test.go:567 +0x314
2024-07-02T12:41:09.1299741Z github.com/hyperledger/fabric/orderer/consensus/smartbft_test.createBFTChainUsingMocks.(*EgressComm_SendConsensus_Call).Run.func21()
2024-07-02T12:41:09.1300235Z /home/runner/work/fabric/fabric/orderer/consensus/smartbft/mocks/egress_comm.go:90 +0xc3
2024-07-02T12:41:09.1300459Z github.com/stretchr/testify/mock.(*Mock).MethodCalled()
2024-07-02T12:41:09.1300958Z /home/runner/work/fabric/fabric/vendor/github.com/stretchr/testify/mock/mock.go:554 +0x129a
2024-07-02T12:41:09.1301151Z github.com/stretchr/testify/mock.(*Mock).Called()
2024-07-02T12:41:09.1301642Z /home/runner/work/fabric/fabric/vendor/github.com/stretchr/testify/mock/mock.go:466 +0x195
2024-07-02T12:41:09.1302070Z github.com/hyperledger/fabric/orderer/consensus/smartbft/mocks.(*EgressComm).SendConsensus()
2024-07-02T12:41:09.1302549Z /home/runner/work/fabric/fabric/orderer/consensus/smartbft/mocks/egress_comm.go:73 +0xe5
2024-07-02T12:41:09.1302987Z github.com/hyperledger-labs/SmartBFT/internal/bft.(*Controller).BroadcastConsensus()
2024-07-02T12:41:09.1303707Z /home/runner/work/fabric/fabric/vendor/github.com/hyperledger-labs/SmartBFT/internal/bft/controller.go:918 +0xcd
2024-07-02T12:41:09.1304128Z github.com/hyperledger-labs/SmartBFT/internal/bft.(*ViewChanger).startViewChange()
2024-07-02T12:41:09.1304871Z /home/runner/work/fabric/fabric/vendor/github.com/hyperledger-labs/SmartBFT/internal/bft/viewchanger.go:384 +0x721
2024-07-02T12:41:09.1305218Z github.com/hyperledger-labs/SmartBFT/internal/bft.(*ViewChanger).run()
2024-07-02T12:41:09.1305953Z /home/runner/work/fabric/fabric/vendor/github.com/hyperledger-labs/SmartBFT/internal/bft/viewchanger.go:216 +0x2c4
2024-07-02T12:41:09.1306341Z github.com/hyperledger-labs/SmartBFT/internal/bft.(*ViewChanger).Start.func1()
2024-07-02T12:41:09.1307079Z /home/runner/work/fabric/fabric/vendor/github.com/hyperledger-labs/SmartBFT/internal/bft/viewchanger.go:157 +0x98
2024-07-02T12:41:09.1307085Z
2024-07-02T12:41:09.1307209Z Goroutine 1298 (running) created at:
2024-07-02T12:41:09.1307563Z github.com/hyperledger-labs/SmartBFT/internal/bft.(*Controller).Start()
2024-07-02T12:41:09.1308288Z /home/runner/work/fabric/fabric/vendor/github.com/hyperledger-labs/SmartBFT/internal/bft/controller.go:808 +0xb13
2024-07-02T12:41:09.1308714Z github.com/hyperledger-labs/SmartBFT/pkg/consensus.(*Consensus).startComponents()
2024-07-02T12:41:09.1309503Z /home/runner/work/fabric/fabric/vendor/github.com/hyperledger-labs/SmartBFT/pkg/consensus/consensus.go:519 +0xf0
2024-07-02T12:41:09.1309863Z github.com/hyperledger-labs/SmartBFT/pkg/consensus.(*Consensus).Start()
2024-07-02T12:41:09.1310600Z /home/runner/work/fabric/fabric/vendor/github.com/hyperledger-labs/SmartBFT/pkg/consensus/consensus.go:160 +0x128e
2024-07-02T12:41:09.1310934Z github.com/hyperledger/fabric/orderer/consensus/smartbft.(*BFTChain).Start()
2024-07-02T12:41:09.1311365Z /home/runner/work/fabric/fabric/orderer/consensus/smartbft/chain.go:453 +0x69
2024-07-02T12:41:09.1311708Z github.com/hyperledger/fabric/orderer/consensus/smartbft_test.(*Node).Restart()
2024-07-02T12:41:09.1312296Z /home/runner/work/fabric/fabric/orderer/consensus/smartbft/util_network_test.go:264 +0x205
2024-07-02T12:41:09.1312769Z github.com/hyperledger/fabric/orderer/consensus/smartbft_test.TestAddNodeWhileAnotherNodeIsDown()
2024-07-02T12:41:09.1313234Z /home/runner/work/fabric/fabric/orderer/consensus/smartbft/chain_test.go:335 +0x948
2024-07-02T12:41:09.1313350Z testing.tRunner()
2024-07-02T12:41:09.1313709Z /opt/hostedtoolcache/go/1.22.4/x64/src/testing/testing.go:1689 +0x21e
2024-07-02T12:41:09.1313839Z testing.(*T).Run.gowrap1()
2024-07-02T12:41:09.1314182Z /opt/hostedtoolcache/go/1.22.4/x64/src/testing/testing.go:1742 +0x44
2024-07-02T12:41:09.1314188Z
2024-07-02T12:41:09.1314313Z Goroutine 1295 (running) created at:
2024-07-02T12:41:09.1314677Z github.com/hyperledger-labs/SmartBFT/internal/bft.(*ViewChanger).Start()
2024-07-02T12:41:09.1315417Z /home/runner/work/fabric/fabric/vendor/github.com/hyperledger-labs/SmartBFT/internal/bft/viewchanger.go:154 +0xa67
2024-07-02T12:41:09.1315834Z github.com/hyperledger-labs/SmartBFT/pkg/consensus.(*Consensus).startComponents()
2024-07-02T12:41:09.1316549Z /home/runner/work/fabric/fabric/vendor/github.com/hyperledger-labs/SmartBFT/pkg/consensus/consensus.go:517 +0x85
2024-07-02T12:41:09.1316914Z github.com/hyperledger-labs/SmartBFT/pkg/consensus.(*Consensus).Start()
2024-07-02T12:41:09.1317644Z /home/runner/work/fabric/fabric/vendor/github.com/hyperledger-labs/SmartBFT/pkg/consensus/consensus.go:160 +0x128e
2024-07-02T12:41:09.1317981Z github.com/hyperledger/fabric/orderer/consensus/smartbft.(*BFTChain).Start()
2024-07-02T12:41:09.1318408Z /home/runner/work/fabric/fabric/orderer/consensus/smartbft/chain.go:453 +0x69
2024-07-02T12:41:09.1318763Z github.com/hyperledger/fabric/orderer/consensus/smartbft_test.(*Node).Restart()
2024-07-02T12:41:09.1319262Z /home/runner/work/fabric/fabric/orderer/consensus/smartbft/util_network_test.go:264 +0x205
2024-07-02T12:41:09.1319740Z github.com/hyperledger/fabric/orderer/consensus/smartbft_test.TestAddNodeWhileAnotherNodeIsDown()
2024-07-02T12:41:09.1320197Z /home/runner/work/fabric/fabric/orderer/consensus/smartbft/chain_test.go:335 +0x948
2024-07-02T12:41:09.1320304Z testing.tRunner()
2024-07-02T12:41:09.1320668Z /opt/hostedtoolcache/go/1.22.4/x64/src/testing/testing.go:1689 +0x21e
2024-07-02T12:41:09.1320790Z testing.(*T).Run.gowrap1()
2024-07-02T12:41:09.1321138Z /opt/hostedtoolcache/go/1.22.4/x64/src/testing/testing.go:1742 +0x44
2024-07-02T12:41:09.1321227Z ==================
There was a problem hiding this comment.
yeah it does look like a write-write on the error variable
Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
yacovm
approved these changes
Jul 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.