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.

Sign up for GitHub

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

chore: replace unbuffered channels with buffered channels #1668

Merged
merged 14 commits into from
Jul 9, 2021
Merged

chore: replace unbuffered channels with buffered channels #1668

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6885bae
add buffer to channels
edwardmack Jul 1, 2021
797b0bf
add buffers to channels in tests
edwardmack Jul 1, 2021
0bbd004
Merge branch 'development' into ed/add_channel_buffers
edwardmack Jul 2, 2021
c84976a
remove buffers from channels that shouldn't be buffered
edwardmack Jul 2, 2021
634050b
Merge branch 'development' into ed/add_channel_buffers
edwardmack Jul 2, 2021
8ed5ee6
Merge branch 'development' into ed/add_channel_buffers
edwardmack Jul 6, 2021
39eb00f
added DEFAULT_BUFFERS_SIZE const
edwardmack Jul 6, 2021
504e4f6
Merge branch 'development' into ed/add_channel_buffers
edwardmack Jul 7, 2021
6395b5c
lint
edwardmack Jul 7, 2021
79db38b
Merge branch 'development' into ed/add_channel_buffers
edwardmack Jul 7, 2021
c8936e5
Merge branch 'development' into ed/add_channel_buffers
edwardmack Jul 7, 2021
3e43488
addres comments
edwardmack Jul 8, 2021
5cd262c
Merge branch 'development' into ed/add_channel_buffers
edwardmack Jul 8, 2021
651e07a
Merge branch 'development' into ed/add_channel_buffers
edwardmack Jul 9, 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
2 changes: 1 addition & 1 deletion dot/core/messages_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ func TestService_HandleTransactionMessage(t *testing.T) {
ks.Acco.Insert(kp)

bp := new(MockBlockProducer) // nolint
blockC := make(chan types.Block)
blockC := make(chan types.Block, 100)
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if buffered channels are needed in tests @arijitAD what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if I needed to add them to tests, but I thought I wouldn't hurt....

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. We can keep them unbuffered in tests.

bp.On("GetBlockChannel", nil).Return(blockC)

cfg := &Config{
Expand Down
8 changes: 4 additions & 4 deletions dot/rpc/subscription/listeners_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func TestStorageObserver_Update(t *testing.T) {
}

func TestBlockListener_Listen(t *testing.T) {
notifyChan := make(chan *types.Block)
notifyChan := make(chan *types.Block, 100)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make the value 100 constant and all packages should use it?
It will make it easy to configure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added const DEFAULT_BUFFER_SIZE, and removed the buffers from the channels that are used in tests.

Copy link
Member

Choose a reason for hiding this comment

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

This one could be unbuffered?

mockConnection := &MockWSConnAPI{}
bl := BlockListener{
Channel: notifyChan,
Expand All @@ -95,7 +95,7 @@ func TestBlockListener_Listen(t *testing.T) {
}

func TestBlockFinalizedListener_Listen(t *testing.T) {
notifyChan := make(chan *types.FinalisationInfo)
notifyChan := make(chan *types.FinalisationInfo, 100)
Copy link
Member

Choose a reason for hiding this comment

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

could be unbuffered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

mockConnection := &MockWSConnAPI{}
bfl := BlockFinalizedListener{
channel: notifyChan,
Expand All @@ -121,8 +121,8 @@ func TestBlockFinalizedListener_Listen(t *testing.T) {
}

func TestExtrinsicSubmitListener_Listen(t *testing.T) {
notifyImportedChan := make(chan *types.Block)
notifyFinalizedChan := make(chan *types.FinalisationInfo)
notifyImportedChan := make(chan *types.Block, 100)
notifyFinalizedChan := make(chan *types.FinalisationInfo, 100)

mockConnection := &MockWSConnAPI{}
esl := ExtrinsicSubmitListener{
Expand Down
8 changes: 4 additions & 4 deletions dot/rpc/subscription/websocket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ func (c *WSConn) unsubscribeStorageListener(reqID float64, params interface{}) {

func (c *WSConn) initBlockListener(reqID float64) (uint, error) {
bl := &BlockListener{
Channel: make(chan *types.Block),
Channel: make(chan *types.Block, 100),
wsconn: c,
}

Expand All @@ -283,7 +283,7 @@ func (c *WSConn) initBlockListener(reqID float64) (uint, error) {

func (c *WSConn) initBlockFinalizedListener(reqID float64) (uint, error) {
bfl := &BlockFinalizedListener{
channel: make(chan *types.FinalisationInfo),
channel: make(chan *types.FinalisationInfo, 100),
Copy link
Member

Choose a reason for hiding this comment

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

could use core.DEFAULT_BUFFER_SIZE?

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, updated.

wsconn: c,
}

Expand Down Expand Up @@ -315,10 +315,10 @@ func (c *WSConn) initExtrinsicWatch(reqID float64, params interface{}) (uint, er

// listen for built blocks
esl := &ExtrinsicSubmitListener{
importedChan: make(chan *types.Block),
importedChan: make(chan *types.Block, 100),
wsconn: c,
extrinsic: types.Extrinsic(extBytes),
finalisedChan: make(chan *types.FinalisationInfo),
finalisedChan: make(chan *types.FinalisationInfo, 100),
Copy link
Member

Choose a reason for hiding this comment

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

could use core.DEFAULT_BUFFER_SIZE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

}

if c.BlockAPI == nil {
Expand Down
4 changes: 2 additions & 2 deletions dot/state/block_notify_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func TestImportChannel_Multi(t *testing.T) {

var err error
for i := 0; i < num; i++ {
chs[i] = make(chan *types.Block)
chs[i] = make(chan *types.Block, 100)
ids[i], err = bs.RegisterImportedChannel(chs[i])
require.NoError(t, err)
}
Expand Down Expand Up @@ -122,7 +122,7 @@ func TestFinalizedChannel_Multi(t *testing.T) {

var err error
for i := 0; i < num; i++ {
chs[i] = make(chan *types.FinalisationInfo)
chs[i] = make(chan *types.FinalisationInfo, 100)
ids[i], err = bs.RegisterFinalizedChannel(chs[i])
require.NoError(t, err)
}
Expand Down
6 changes: 3 additions & 3 deletions dot/telemetry/telemetry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func TestHandler_SendMulti(t *testing.T) {
var wg sync.WaitGroup
wg.Add(4)

resultCh = make(chan []byte)
resultCh = make(chan []byte, 100)

go func() {
genesisHash := common.MustHexToHash("0x91b171bb158e2d3848fa23a9f1c25182fb8e20313b2c1eb49219da7a70ce90c3")
Expand Down Expand Up @@ -109,7 +109,7 @@ func TestListenerConcurrency(t *testing.T) {
var wg sync.WaitGroup
wg.Add(qty)

resultCh = make(chan []byte)
resultCh = make(chan []byte, 100)
for i := 0; i < qty; i++ {
go func() {
bestHash := common.Hash{}
Expand All @@ -132,7 +132,7 @@ func TestListenerConcurrency(t *testing.T) {
// this can be useful to see what data is sent to telemetry server
func TestInfiniteListener(t *testing.T) {
t.Skip()
resultCh = make(chan []byte)
resultCh = make(chan []byte, 100)
for data := range resultCh {
fmt.Printf("Data %s\n", data)
}
Expand Down
2 changes: 1 addition & 1 deletion lib/grandpa/grandpa.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ func NewService(cfg *Config) (*Service, error) {
justification: make(map[uint64][]*SignedPrecommit),
head: head,
in: make(chan GrandpaMessage, 128),
resumed: make(chan struct{}),
resumed: make(chan struct{}, 100),
Copy link
Contributor

Choose a reason for hiding this comment

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

you can leave this as unbuffered since it's a channel that's just created and closed on resume

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

network: cfg.Network,
finalisedCh: finalisedCh,
finalisedChID: fid,
Expand Down
2 changes: 1 addition & 1 deletion tests/rpc/rpc_03-chain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ func callWebsocket(t *testing.T, test *testCase) {

done := make(chan struct{})

vals := make(chan []byte)
vals := make(chan []byte, 100)
go wsListener(t, ws, vals, done, len(test.expected.([]interface{})))

err = ws.WriteMessage(websocket.TextMessage, []byte(`{
Expand Down