fix: deadlock on retrieving WebTransport addresses #9857
Conversation
| libp2p.ListenAddrStrings(addresses...), | ||
| }, | ||
| } | ||
| log.Infof("Swarm listening at: %s", addrs) |
There was a problem hiding this comment.
Is this important? We already print Swarm listening on in daemon.go:769. Usage of log.Infof instead of fmt.Println seems to be the difference here, and this wouldn't be printed by default unless the user has logging enabled. If there is a reason to keep this, I suggest the following:
diff --git a/core/node/libp2p/hostopt.go b/core/node/libp2p/hostopt.go
index 74d6e5723..f8bdbed30 100644
--- a/core/node/libp2p/hostopt.go
+++ b/core/node/libp2p/hostopt.go
@@ -20,5 +20,11 @@ func constructPeerHost(id peer.ID, ps peerstore.Peerstore, options ...libp2p.Opt
return nil, fmt.Errorf("missing private key for node ID: %s", id.Pretty())
}
options = append([]libp2p.Option{libp2p.Identity(pkey), libp2p.Peerstore(ps)}, options...)
- return libp2p.New(options...)
+ h, err := libp2p.New(options...)
+ if err != nil {
+ return nil, err
+ }
+
+ log.Infof("Swarm listening at: %s", h.Network().ListenAddresses())
+ return h, nil
}There was a problem hiding this comment.
From what I can tell we shouldn't need this log info any more given we print it out on daemon run, unless there is code only reading from the log. From what I can tell this writing the listen addresses twice has been there for a very long time with no note indicating this was intentional in #3948.
I think if we want to keep the log there putting it where @hacdias suggested is reasonable, or we could move it next to the Printf so that it's more obvious what's going on.
Co-Authored-By: Marco Polo <[email protected]>
| libp2p.ListenAddrStrings(addresses...), | ||
| }, | ||
| } | ||
| log.Infof("Swarm listening at: %s", addrs) |
There was a problem hiding this comment.
From what I can tell we shouldn't need this log info any more given we print it out on daemon run, unless there is code only reading from the log. From what I can tell this writing the listen addresses twice has been there for a very long time with no note indicating this was intentional in #3948.
I think if we want to keep the log there putting it where @hacdias suggested is reasonable, or we could move it next to the Printf so that it's more obvious what's going on.
| addrs, err := host.Network().InterfaceListenAddresses() | ||
| if err != nil { | ||
| return err | ||
| func ListenOn(addresses []string) interface{} { |
There was a problem hiding this comment.
I like that we're using the libp2p constructor now to pass listen addresses. However, I'm a little nervous about there being ancient race conditions that emerge here 1a3752b.
Hopefully this shouldn't be a problem any more given that it's highly unintuitive behavior and that if people used those code paths without copying kubo code they'd unlikely do the same thing as was done here.
@Jorropo you've been looking through the Bitswap code more recently than most any thoughts on if those race conditions are likely still present?
Note: lotus probably has a similar issue here https://github.com/filecoin-project/lotus/blob/e6c8072f505f724f869861ca0430101fc87286c0/node/modules/lp2p/addrs.go#L84 that should be resolved when they upgrade go-libp2p in filecoin-project/lotus#10671.
Co-authored-by: Marco Polo <[email protected]>
Fixes #9838.
master)Notes: