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

go/p2p/rpc: Refactor rpc calls #5007

Merged
merged 4 commits into from
Oct 31, 2022
Merged

Conversation

peternose
Copy link
Contributor

@peternose peternose commented Oct 29, 2022

Peer selection is very tightly coupled with RPC client as peer manager is constructed inside client's constructor. We also miss simple RPC calls which contact exactly one peer.

Test

Memory leak was check with the following code, which prints 1 and 101, meaning that 100 go routines leaked.

package main

import (
	"fmt"
	"runtime"
	"time"

	"github.com/eapache/channels"
)

func main() {
	fmt.Println(runtime.NumGoroutine())
	for i := 0; i < 100; i++ {
		leak()
	}
	time.Sleep(time.Minute)
	fmt.Println(runtime.NumGoroutine())
}

func leak() {
	n := 10
	var chs []channels.SimpleOutChannel

	for i := 0; i < n; i++ {
		ch := channels.NewNativeChannel(channels.BufferCap(1))
		defer close(ch)

		chs = append(chs, ch)
		ch <- i
	}

	ch := channels.NewNativeChannel(channels.None)
	channels.Multiplex(ch, chs...)

	for v := range ch.Out() {
		if v.(int) < n/2 {
			break
		}
	}
}

@peternose peternose force-pushed the peternose/internal/refactor-rpc-calls branch 2 times, most recently from 6ca42e4 to b797444 Compare October 29, 2022 15:49
@peternose peternose marked this pull request as ready for review October 29, 2022 16:22
@codecov
Copy link

codecov bot commented Oct 29, 2022

Codecov Report

Merging #5007 (b548870) into master (a888835) will decrease coverage by 0.06%.
The diff coverage is 89.69%.

@@            Coverage Diff             @@
##           master    #5007      +/-   ##
==========================================
- Coverage   66.68%   66.62%   -0.07%     
==========================================
  Files         478      478              
  Lines       51800    51868      +68     
==========================================
+ Hits        34544    34556      +12     
- Misses      12983    13041      +58     
+ Partials     4273     4271       -2     
Impacted Files Coverage Δ
go/p2p/rpc/server.go 78.72% <ø> (ø)
go/p2p/rpc/nop.go 6.66% <20.00%> (+6.66%) ⬆️
go/p2p/rpc/client.go 79.73% <87.38%> (+0.11%) ⬆️
go/p2p/rpc/peermgmt.go 82.35% <93.75%> (+2.97%) ⬆️
go/p2p/txsync/client.go 72.97% <100.00%> (+4.22%) ⬆️
go/worker/keymanager/p2p/client.go 81.17% <100.00%> (+0.22%) ⬆️
go/worker/storage/p2p/pub/client.go 84.61% <100.00%> (+3.66%) ⬆️
go/worker/storage/p2p/sync/client.go 89.28% <100.00%> (-2.02%) ⬇️
go/roothash/api/sanity_check.go 40.00% <0.00%> (-20.00%) ⬇️
go/worker/storage/committee/utils.go 92.30% <0.00%> (-7.70%) ⬇️
... and 37 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Can you also backport the memory leak and peer scoring fixes?

go/p2p/rpc/client.go Outdated Show resolved Hide resolved
go/p2p/rpc/client.go Outdated Show resolved Hide resolved
go/p2p/rpc/client.go Show resolved Hide resolved
When method CallMulti finishes early, the requests in progress are canceled
and unfairly recorded as failed. Furthermore, the result channel is never
cleared neither closed which leaves one go routine hanging.
@peternose peternose force-pushed the peternose/internal/refactor-rpc-calls branch from 1aefc95 to b548870 Compare October 30, 2022 21:08
@peternose peternose merged commit 9329c4e into master Oct 31, 2022
@peternose peternose deleted the peternose/internal/refactor-rpc-calls branch October 31, 2022 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants