Skip to content
This repository has been archived by the owner on Jan 3, 2023. It is now read-only.

Remove redundant locks in subnet manager #240

Closed
adlrocha opened this issue Aug 25, 2022 · 2 comments
Closed

Remove redundant locks in subnet manager #240

adlrocha opened this issue Aug 25, 2022 · 2 comments
Labels
good first issue Good for newcomers

Comments

@adlrocha
Copy link
Collaborator

The use of the new subnetsLock may be more efficient and defeat the purpose of Service.lk:

eudico/chain/consensus/hierarchical/subnet/submgr/manager.go

Lines 830 to 831 in 12fceca

s.subnetsLock.Lock()
defer s.subnetsLock.Unlock()

We need to re-evaluate the code to see if we can remove any of them, they seem redundant.

//cc @dnkolegov
@adlrocha adlrocha added the good first issue Good for newcomers label Aug 25, 2022
@dnkolegov
Copy link
Collaborator

I did a very simple experiment. I removed lk lock and subnetLock. Most of the time the tests worked as usual. But sometimes the same data race happened.
We should totally reconsider our locking policy for the subnet manager.

==================
WARNING: DATA RACE
Read at 0x00c0001c7c28 by goroutine 1080:
  github.com/filecoin-project/lotus/chain/consensus/hierarchical/subnet/submgr.(*Service).getAPI()
      /Users/alpha/Projects/eudico/chain/consensus/hierarchical/subnet/submgr/manager.go:808 +0x1d4
  github.com/filecoin-project/lotus/chain/consensus/hierarchical/subnet/submgr.(*Service).GetSubnetAPI()
      /Users/alpha/Projects/eudico/chain/consensus/hierarchical/subnet/submgr/manager.go:832 +0x54
  github.com/filecoin-project/lotus/chain/consensus/hierarchical/subnet/submgr.(*Service).GetSCAState()
      /Users/alpha/Projects/eudico/chain/consensus/hierarchical/subnet/submgr/manager.go:840 +0x68
  github.com/filecoin-project/lotus/chain/consensus/common/crossmsg.GetSCAState()
      /Users/alpha/Projects/eudico/chain/consensus/common/crossmsg/crossmsg.go:250 +0x694
  github.com/filecoin-project/lotus/chain/consensus/common.checkBlockMessages()
      /Users/alpha/Projects/eudico/chain/consensus/common/cns_validations.go:309 +0xfdc
  github.com/filecoin-project/lotus/chain/consensus/common.CheckMsgsWithoutBlockSig.func1()
      /Users/alpha/Projects/eudico/chain/consensus/common/cns_validations.go:112 +0x128
  github.com/Gurpartap/async.Err.func1()
      /Users/alpha/go/pkg/mod/github.com/!gurpartap/async@v0.0.0-20180927173644-4f7f499dd9ee/error.go:29 +0x74

Previous write at 0x00c0001c7c28 by goroutine 1085:
  [failed to restore the stack]

Goroutine 1080 (running) created at:
  github.com/Gurpartap/async.Err()
      /Users/alpha/go/pkg/mod/github.com/!gurpartap/async@v0.0.0-20180927173644-4f7f499dd9ee/error.go:27 +0x158
  github.com/filecoin-project/lotus/chain/consensus/common.CheckMsgsWithoutBlockSig()
      /Users/alpha/Projects/eudico/chain/consensus/common/cns_validations.go:107 +0x2b0
  github.com/filecoin-project/lotus/chain/consensus/mir.(*Mir).ValidateBlock()
      /Users/alpha/Projects/eudico/chain/consensus/mir/consensus.go:129 +0x6a0
  github.com/filecoin-project/lotus/chain.(*Syncer).ValidateBlock()
      /Users/alpha/Projects/eudico/chain/sync.go:628 +0x214
  github.com/filecoin-project/lotus/chain.(*Syncer).ValidateTipSet.func1()
      /Users/alpha/Projects/eudico/chain/sync.go:577 +0x74
  github.com/Gurpartap/async.Err.func1()
      /Users/alpha/go/pkg/mod/github.com/!gurpartap/async@v0.0.0-20180927173644-4f7f499dd9ee/error.go:29 +0x74

Goroutine 1085 (running) created at:
  github.com/filecoin-project/go-jsonrpc.(*wsConn).handleCall()
      /Users/alpha/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.1.5/websocket.go:424 +0x5f8
  github.com/filecoin-project/go-jsonrpc.(*wsConn).handleFrame()
      /Users/alpha/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.1.5/websocket.go:443 +0x2a0
  github.com/filecoin-project/go-jsonrpc.(*wsConn).handleWsConn()
      /Users/alpha/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.1.5/websocket.go:614 +0x8a0
  github.com/filecoin-project/go-jsonrpc.(*RPCServer).handleWS()
      /Users/alpha/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.1.5/server.go:73 +0x440
  github.com/filecoin-project/go-jsonrpc.(*RPCServer).ServeHTTP()
      /Users/alpha/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.1.5/server.go:87 +0xf0
  github.com/gorilla/mux.(*Router).ServeHTTP()
      /Users/alpha/go/pkg/mod/github.com/gorilla/mux@v1.8.0/mux.go:210 +0x2a0
  net/http.serverHandler.ServeHTTP()
      /opt/homebrew/Cellar/go/1.18.2/libexec/src/net/http/server.go:2916 +0x6cc
  net/http.(*conn).serve()
      /opt/homebrew/Cellar/go/1.18.2/libexec/src/net/http/server.go:1966 +0x8e0
  net/http.(*Server).Serve.func3()
      /opt/homebrew/Cellar/go/1.18.2/libexec/src/net/http/server.go:3071 +0x50
==================
==================
WARNING: DATA RACE
Read at 0x00c003cc0d58 by goroutine 1080:
  github.com/filecoin-project/lotus/chain/consensus/hierarchical/subnet/submgr.(*API).StateGetActor()
      <autogenerated>:1 +0x5c
  github.com/filecoin-project/lotus/chain/consensus/hierarchical/subnet/submgr.(*Service).GetSCAState()
      /Users/alpha/Projects/eudico/chain/consensus/hierarchical/subnet/submgr/manager.go:845 +0x140
  github.com/filecoin-project/lotus/chain/consensus/common/crossmsg.GetSCAState()
      /Users/alpha/Projects/eudico/chain/consensus/common/crossmsg/crossmsg.go:250 +0x694
  github.com/filecoin-project/lotus/chain/consensus/common.checkBlockMessages()
      /Users/alpha/Projects/eudico/chain/consensus/common/cns_validations.go:309 +0xfdc
  github.com/filecoin-project/lotus/chain/consensus/common.CheckMsgsWithoutBlockSig.func1()
      /Users/alpha/Projects/eudico/chain/consensus/common/cns_validations.go:112 +0x128
  github.com/Gurpartap/async.Err.func1()
      /Users/alpha/go/pkg/mod/github.com/!gurpartap/async@v0.0.0-20180927173644-4f7f499dd9ee/error.go:29 +0x74

Previous write at 0x00c003cc0d58 by goroutine 1085:
  [failed to restore the stack]

Goroutine 1080 (running) created at:
  github.com/Gurpartap/async.Err()
      /Users/alpha/go/pkg/mod/github.com/!gurpartap/async@v0.0.0-20180927173644-4f7f499dd9ee/error.go:27 +0x158
  github.com/filecoin-project/lotus/chain/consensus/common.CheckMsgsWithoutBlockSig()
      /Users/alpha/Projects/eudico/chain/consensus/common/cns_validations.go:107 +0x2b0
  github.com/filecoin-project/lotus/chain/consensus/mir.(*Mir).ValidateBlock()
      /Users/alpha/Projects/eudico/chain/consensus/mir/consensus.go:129 +0x6a0
  github.com/filecoin-project/lotus/chain.(*Syncer).ValidateBlock()
      /Users/alpha/Projects/eudico/chain/sync.go:628 +0x214
  github.com/filecoin-project/lotus/chain.(*Syncer).ValidateTipSet.func1()
      /Users/alpha/Projects/eudico/chain/sync.go:577 +0x74
  github.com/Gurpartap/async.Err.func1()
      /Users/alpha/go/pkg/mod/github.com/!gurpartap/async@v0.0.0-20180927173644-4f7f499dd9ee/error.go:29 +0x74

Goroutine 1085 (running) created at:
  github.com/filecoin-project/go-jsonrpc.(*wsConn).handleCall()
      /Users/alpha/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.1.5/websocket.go:424 +0x5f8
  github.com/filecoin-project/go-jsonrpc.(*wsConn).handleFrame()
      /Users/alpha/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.1.5/websocket.go:443 +0x2a0
  github.com/filecoin-project/go-jsonrpc.(*wsConn).handleWsConn()
      /Users/alpha/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.1.5/websocket.go:614 +0x8a0
  github.com/filecoin-project/go-jsonrpc.(*RPCServer).handleWS()
      /Users/alpha/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.1.5/server.go:73 +0x440
  github.com/filecoin-project/go-jsonrpc.(*RPCServer).ServeHTTP()
      /Users/alpha/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.1.5/server.go:87 +0xf0
  github.com/gorilla/mux.(*Router).ServeHTTP()
      /Users/alpha/go/pkg/mod/github.com/gorilla/mux@v1.8.0/mux.go:210 +0x2a0
  net/http.serverHandler.ServeHTTP()
      /opt/homebrew/Cellar/go/1.18.2/libexec/src/net/http/server.go:2916 +0x6cc
  net/http.(*conn).serve()
      /opt/homebrew/Cellar/go/1.18.2/libexec/src/net/http/server.go:1966 +0x8e0
  net/http.(*Server).Serve.func3()
      /opt/homebrew/Cellar/go/1.18.2/libexec/src/net/http/server.go:3071 +0x50
==================
==================
WARNING: DATA RACE
Read at 0x00c003cc08e8 by goroutine 1080:
  github.com/filecoin-project/lotus/chain/consensus/hierarchical/subnet/submgr.(*API).ChainReadObj()
      <autogenerated>:1 +0x50
  github.com/filecoin-project/lotus/blockstore.(*apiBlockstore).Get()
      /Users/alpha/Projects/eudico/blockstore/api.go:37 +0x6c
  github.com/filecoin-project/lotus/blockstore.(*adaptedBlockstore).Get()
      <autogenerated>:1 +0x74
  github.com/ipfs/go-ipld-cbor.(*BasicIpldStore).Get()
      /Users/alpha/go/pkg/mod/github.com/ipfs/go-ipld-cbor@v0.0.6/store.go:63 +0x180
  github.com/filecoin-project/lotus/chain/consensus/hierarchical/subnet/submgr.(*Service).GetSCAState()
      /Users/alpha/Projects/eudico/chain/consensus/hierarchical/subnet/submgr/manager.go:851 +0x400
  github.com/filecoin-project/lotus/chain/consensus/common/crossmsg.GetSCAState()
      /Users/alpha/Projects/eudico/chain/consensus/common/crossmsg/crossmsg.go:250 +0x694
  github.com/filecoin-project/lotus/chain/consensus/common.checkBlockMessages()
      /Users/alpha/Projects/eudico/chain/consensus/common/cns_validations.go:309 +0xfdc
  github.com/filecoin-project/lotus/chain/consensus/common.CheckMsgsWithoutBlockSig.func1()
      /Users/alpha/Projects/eudico/chain/consensus/common/cns_validations.go:112 +0x128
  github.com/Gurpartap/async.Err.func1()
      /Users/alpha/go/pkg/mod/github.com/!gurpartap/async@v0.0.0-20180927173644-4f7f499dd9ee/error.go:29 +0x74

Previous write at 0x00c003cc08e8 by goroutine 1085:
  [failed to restore the stack]

Goroutine 1080 (running) created at:
  github.com/Gurpartap/async.Err()
      /Users/alpha/go/pkg/mod/github.com/!gurpartap/async@v0.0.0-20180927173644-4f7f499dd9ee/error.go:27 +0x158
  github.com/filecoin-project/lotus/chain/consensus/common.CheckMsgsWithoutBlockSig()
      /Users/alpha/Projects/eudico/chain/consensus/common/cns_validations.go:107 +0x2b0
  github.com/filecoin-project/lotus/chain/consensus/mir.(*Mir).ValidateBlock()
      /Users/alpha/Projects/eudico/chain/consensus/mir/consensus.go:129 +0x6a0
  github.com/filecoin-project/lotus/chain.(*Syncer).ValidateBlock()
      /Users/alpha/Projects/eudico/chain/sync.go:628 +0x214
  github.com/filecoin-project/lotus/chain.(*Syncer).ValidateTipSet.func1()
      /Users/alpha/Projects/eudico/chain/sync.go:577 +0x74
  github.com/Gurpartap/async.Err.func1()
      /Users/alpha/go/pkg/mod/github.com/!gurpartap/async@v0.0.0-20180927173644-4f7f499dd9ee/error.go:29 +0x74

Goroutine 1085 (running) created at:
  github.com/filecoin-project/go-jsonrpc.(*wsConn).handleCall()
      /Users/alpha/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.1.5/websocket.go:424 +0x5f8
  github.com/filecoin-project/go-jsonrpc.(*wsConn).handleFrame()
      /Users/alpha/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.1.5/websocket.go:443 +0x2a0
  github.com/filecoin-project/go-jsonrpc.(*wsConn).handleWsConn()
      /Users/alpha/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.1.5/websocket.go:614 +0x8a0
  github.com/filecoin-project/go-jsonrpc.(*RPCServer).handleWS()
      /Users/alpha/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.1.5/server.go:73 +0x440
  github.com/filecoin-project/go-jsonrpc.(*RPCServer).ServeHTTP()
      /Users/alpha/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.1.5/server.go:87 +0xf0
  github.com/gorilla/mux.(*Router).ServeHTTP()
      /Users/alpha/go/pkg/mod/github.com/gorilla/mux@v1.8.0/mux.go:210 +0x2a0
  net/http.serverHandler.ServeHTTP()
      /opt/homebrew/Cellar/go/1.18.2/libexec/src/net/http/server.go:2916 +0x6cc
  net/http.(*conn).serve()
      /opt/homebrew/Cellar/go/1.18.2/libexec/src/net/http/server.go:1966 +0x8e0
  net/http.(*Server).Serve.func3()
      /opt/homebrew/Cellar/go/1.18.2/libexec/src/net/http/server.go:3071 +0x50
==================

@dnkolegov dnkolegov mentioned this issue Aug 25, 2022
Closed
@dnkolegov
Copy link
Collaborator

It will be fixed via these commits:

@adlrocha adlrocha mentioned this issue Aug 30, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@dnkolegov @adlrocha