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

fix: retry user agent get from libp2p peerstore #2482

Merged
merged 6 commits into from
Sep 8, 2021
Merged

fix: retry user agent get from libp2p peerstore #2482

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0636a59
chore: return errors by libp2p peer user agent function
janos Sep 7, 2021
601c7c0
chore: run multiple times
acud Sep 8, 2021
46bb19d
chore: print to stdout
acud Sep 8, 2021
23a1107
chore: retry libp2p usera gent get on peerstore
janos Sep 8, 2021
642f07e
chore: remove debugging code
janos Sep 8, 2021
5fab077
chore: pass context to peer user agent function
janos Sep 8, 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
14 changes: 13 additions & 1 deletion pkg/p2p/libp2p/libp2p.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -884,7 +884,19 @@ func (s *Service) Ping(ctx context.Context, addr ma.Multiaddr) (rtt time.Duratio
// provides it. It ignores the default libp2p user agent string
// "github.com/libp2p/go-libp2p" and returns empty string in that case.
func (s *Service) peerUserAgent(peerID libp2ppeer.ID) string {
v, err := s.host.Peerstore().Get(peerID, "AgentVersion")
var (
v interface{}
err error
)
// Peerstore may not contain all keys and values right after the connections is created.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: *connection

// This retry mechanism ensures more reliable user agent propagation.
for deadline := time.Now().Add(2 * time.Second); time.Now().Before(deadline); {
Copy link
Member

Choose a reason for hiding this comment

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

my only question would be: could it be that the retry mechanism would make some context with deadline in the enclosing context expire?
example:

func (k *Kad) connect(ctx context.Context, peer swarm.Address, ma ma.Multiaddr) error {
	ctx, cancel := context.WithTimeout(ctx, peerConnectionAttemptTimeout)
	defer cancel()

	switch i, err := k.p2p.Connect(ctx, ma); {

kademlia dial has a 5 seconds timeout in this case. so the user agent printing might delay this and make it expire. I wonder if we could either decrease this timespan to be shorter so that it becomes insignificant comparing to the total connection context deadline or simply do this polling in a different goroutine and print it eventually in an unrelated logline that would have the overlay and user agent together.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion. I've added context handling to the function.

v, err = s.host.Peerstore().Get(peerID, "AgentVersion")
if err == nil {
break
}
time.Sleep(50 * time.Millisecond)
}
if err != nil {
// error is ignored as user agent is informative only
return ""
Expand Down