Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: confirm block import notifier is closed properly #1736

Merged
merged 40 commits into from
Sep 24, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
0a59cb3
add TODOs to identify where block imported channel is handled
edwardmack Aug 17, 2021
a8cffd8
Merge branch 'development' into ed/fix_send_on_closed_channel
edwardmack Aug 18, 2021
30338f7
added comments for imported channels
edwardmack Aug 18, 2021
6ed9d63
create constructor for listeners
edwardmack Aug 19, 2021
0bc1f97
Merge branch 'development' into ed/fix_send_on_closed_channel
edwardmack Aug 23, 2021
2ae9572
added close channel to defer in listen
edwardmack Aug 23, 2021
0f95d68
Merge branch 'development' into ed/fix_send_on_closed_channel
edwardmack Aug 25, 2021
2b16fa6
move imported chan to block_notify
edwardmack Aug 26, 2021
7f10eec
Merge branch 'development' into ed/fix_send_on_closed_channel
edwardmack Aug 26, 2021
5f1c2a6
Merge branch 'development' into ed/fix_send_on_closed_channel
edwardmack Aug 30, 2021
2b512b2
remove comments, lint
edwardmack Aug 30, 2021
e3bd220
handle lint issues
edwardmack Aug 30, 2021
fd6d7cc
replace imported channel map with sync.Map
edwardmack Aug 30, 2021
125b68a
fix mocks in listeners test
edwardmack Aug 30, 2021
ee0fc7b
Merge branch 'development' into ed/fix_send_on_closed_channel
edwardmack Aug 31, 2021
6ca1a8b
fix mock functions for new imported notification channel
edwardmack Aug 31, 2021
d3eaa06
fix deep source issues
edwardmack Aug 31, 2021
41bb429
add debugging printf
edwardmack Sep 1, 2021
f0f76f1
Merge branch 'development' into ed/fix_send_on_closed_channel
edwardmack Sep 1, 2021
0164305
remove sync.Pool, and sync.Map
edwardmack Sep 2, 2021
b7f658f
handle channel closing
edwardmack Sep 3, 2021
74f094d
add sleep before close
edwardmack Sep 3, 2021
8a49ae7
remove channel close
edwardmack Sep 3, 2021
35180df
run go imported
edwardmack Sep 3, 2021
faf2664
defer importedLock unlock
edwardmack Sep 7, 2021
db397cd
wrap notifier channel in struct
edwardmack Sep 8, 2021
4b7d341
Merge branch 'development' into ed/fix_send_on_closed_channel
edwardmack Sep 9, 2021
d5d0d57
store channel by interface{} key
edwardmack Sep 9, 2021
f72e744
Merge branch 'ed/fix_send_on_closed_channel' of https://github.com/Ch…
edwardmack Sep 9, 2021
a478d5c
update storage key for imported block listeners
edwardmack Sep 9, 2021
d5754d9
Merge branch 'development' into ed/fix_send_on_closed_channel
edwardmack Sep 9, 2021
8b8066c
refacter GetImportedBlockNotifierChannel arugments
edwardmack Sep 10, 2021
1acfba7
GetImportedBlockNotifierChannel doesn't return error, fixed related test
edwardmack Sep 10, 2021
ce706cc
Merge branch 'development' into ed/fix_send_on_closed_channel
edwardmack Sep 20, 2021
515391d
Merge branch 'development' into ed/fix_send_on_closed_channel
edwardmack Sep 21, 2021
8ad895c
remove un-needed comments
edwardmack Sep 21, 2021
6b46fcd
Merge branch 'development' into ed/fix_send_on_closed_channel
edwardmack Sep 23, 2021
6bfb77d
remove close for FinalisedChannel listener
edwardmack Sep 23, 2021
48f6db0
added test for free imported channel
edwardmack Sep 23, 2021
8b30110
add mocks paths to .deepsource exclude_patterns
edwardmack Sep 24, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .deepsource.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ exclude_patterns = [
"dot/config/**/*",
"dot/rpc/modules/test_data",
"lib/runtime/test_data",
"**/*_test.go"
"**/*_test.go",
"**/mocks/*",
"**/mock_*"
]

[[analyzers]]
Expand Down
4 changes: 2 additions & 2 deletions dot/core/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ type BlockState interface {
GetSlotForBlock(common.Hash) (uint64, error)
GetFinalisedHeader(uint64, uint64) (*types.Header, error)
GetFinalisedHash(uint64, uint64) (common.Hash, error)
RegisterImportedChannel(ch chan<- *types.Block) (byte, error)
UnregisterImportedChannel(id byte)
GetImportedBlockNotifierChannel() chan *types.Block
FreeImportedBlockNotifierChannel(ch chan *types.Block)
RegisterFinalizedChannel(ch chan<- *types.FinalisationInfo) (byte, error)
UnregisterFinalisedChannel(id byte)
HighestCommonAncestor(a, b common.Hash) (common.Hash, error)
Expand Down
61 changes: 21 additions & 40 deletions dot/core/mocks/block_state.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 3 additions & 9 deletions dot/digest/digest.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ type Handler struct {

// block notification channels
imported chan *types.Block
noot marked this conversation as resolved.
Show resolved Hide resolved
importedID byte
finalised chan *types.FinalisationInfo
finalisedID byte

Expand All @@ -74,12 +73,9 @@ type resume struct {

// NewHandler returns a new Handler
func NewHandler(blockState BlockState, epochState EpochState, grandpaState GrandpaState) (*Handler, error) {
imported := make(chan *types.Block, 16)
imported := blockState.GetImportedBlockNotifierChannel()

finalised := make(chan *types.FinalisationInfo, 16)
iid, err := blockState.RegisterImportedChannel(imported)
if err != nil {
return nil, err
}

fid, err := blockState.RegisterFinalizedChannel(finalised)
if err != nil {
Expand All @@ -95,7 +91,6 @@ func NewHandler(blockState BlockState, epochState EpochState, grandpaState Grand
epochState: epochState,
grandpaState: grandpaState,
imported: imported,
importedID: iid,
finalised: finalised,
finalisedID: fid,
}, nil
Expand All @@ -111,9 +106,8 @@ func (h *Handler) Start() error {
// Stop stops the Handler
func (h *Handler) Stop() error {
h.cancel()
h.blockState.UnregisterImportedChannel(h.importedID)
h.blockState.FreeImportedBlockNotifierChannel(h.imported)
h.blockState.UnregisterFinalisedChannel(h.finalisedID)
close(h.imported)
close(h.finalised)
Comment on lines 110 to 111
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be good to update the finalised notifiers with the new architecture as well, as it has the same problem. can open another issue or do this in this PR, up to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll update once the architecture is set.

return nil
}
Expand Down
4 changes: 2 additions & 2 deletions dot/digest/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ import (
// BlockState interface for block state methods
type BlockState interface {
BestBlockHeader() (*types.Header, error)
RegisterImportedChannel(ch chan<- *types.Block) (byte, error)
UnregisterImportedChannel(id byte)
GetImportedBlockNotifierChannel() chan *types.Block
FreeImportedBlockNotifierChannel(ch chan *types.Block)
RegisterFinalizedChannel(ch chan<- *types.FinalisationInfo) (byte, error)
UnregisterFinalisedChannel(id byte)
}
Expand Down
3 changes: 1 addition & 2 deletions dot/rpc/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,7 @@ func (h *HTTPServer) Stop() error {
case *subscription.StorageObserver:
h.serverConfig.StorageAPI.UnregisterStorageObserver(v)
case *subscription.BlockListener:
h.serverConfig.BlockAPI.UnregisterImportedChannel(v.ChanID)
close(v.Channel)
h.serverConfig.BlockAPI.FreeImportedBlockNotifierChannel(v.Channel)
}
}

Expand Down
4 changes: 2 additions & 2 deletions dot/rpc/modules/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ type BlockAPI interface {
GetHighestFinalisedHash() (common.Hash, error)
HasJustification(hash common.Hash) (bool, error)
GetJustification(hash common.Hash) ([]byte, error)
RegisterImportedChannel(ch chan<- *types.Block) (byte, error)
UnregisterImportedChannel(id byte)
GetImportedBlockNotifierChannel() chan *types.Block
FreeImportedBlockNotifierChannel(ch chan *types.Block)
RegisterFinalizedChannel(ch chan<- *types.FinalisationInfo) (byte, error)
UnregisterFinalisedChannel(id byte)
SubChain(start, end common.Hash) ([]common.Hash, error)
Expand Down
8 changes: 5 additions & 3 deletions dot/rpc/modules/api_mocks.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package modules

import (
modulesmocks "github.com/ChainSafe/gossamer/dot/rpc/modules/mocks"
"github.com/ChainSafe/gossamer/dot/types"
"github.com/ChainSafe/gossamer/lib/common"
runtimemocks "github.com/ChainSafe/gossamer/lib/runtime/mocks"
"github.com/stretchr/testify/mock"
Expand All @@ -21,15 +22,16 @@ func NewMockStorageAPI() *modulesmocks.MockStorageAPI {
}

// NewMockBlockAPI creates and return an rpc BlockAPI interface mock
func NewMockBlockAPI() *modulesmocks.BlockAPI {
m := new(modulesmocks.BlockAPI)
func NewMockBlockAPI() *modulesmocks.MockBlockAPI {
m := new(modulesmocks.MockBlockAPI)
m.On("GetHeader", mock.AnythingOfType("common.Hash")).Return(nil, nil)
m.On("BestBlockHash").Return(common.Hash{})
m.On("GetBlockByHash", mock.AnythingOfType("common.Hash")).Return(nil, nil)
m.On("GetBlockHash", mock.AnythingOfType("*big.Int")).Return(nil, nil)
m.On("GetFinalisedHash", mock.AnythingOfType("uint64"), mock.AnythingOfType("uint64")).Return(common.Hash{}, nil)
m.On("GetHighestFinalisedHash").Return(common.Hash{}, nil)
m.On("RegisterImportedChannel", mock.AnythingOfType("chan<- *types.Block")).Return(byte(0), nil)
m.On("GetImportedBlockNotifierChannel").Return(make(chan *types.Block, 5))
m.On("FreeImportedBlockNotifierChannel", mock.AnythingOfType("chan *types.Block"))
m.On("UnregisterImportedChannel", mock.AnythingOfType("uint8"))
m.On("RegisterFinalizedChannel", mock.AnythingOfType("chan<- *types.FinalisationInfo")).Return(byte(0), nil)
m.On("UnregisterFinalizedChannel", mock.AnythingOfType("uint8"))
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dot/rpc/modules/system_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ func TestSyncState(t *testing.T) {
Number: big.NewInt(int64(49)),
}

blockapiMock := new(mocks.BlockAPI)
blockapiMock := new(mocks.MockBlockAPI)
blockapiMock.On("BestBlockHash").Return(fakeCommonHash)
blockapiMock.On("GetHeader", fakeCommonHash).Return(fakeHeader, nil).Once()

Expand Down
Loading