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: deadlock on retrieving WebTransport addresses #9857

Merged
merged 3 commits into from
May 8, 2023
Merged

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented May 5, 2023

@hacdias hacdias changed the title wip: try marco's fx foo fix: deadlock on retrieving WebTransport addresses May 5, 2023
}
log.Infof("Swarm listening at: %s", addrs)
Copy link
Member Author

@hacdias hacdias May 5, 2023

Choose a reason for hiding this comment

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

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
 }

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@hacdias hacdias self-assigned this May 5, 2023
@hacdias hacdias marked this pull request as ready for review May 5, 2023 11:48
@hacdias hacdias requested a review from a team as a code owner May 5, 2023 11:48
test/cli/transports_test.go Outdated Show resolved Hide resolved
}
log.Infof("Swarm listening at: %s", addrs)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

core/mock/mock.go Show resolved Hide resolved
addrs, err := host.Network().InterfaceListenAddresses()
if err != nil {
return err
func ListenOn(addresses []string) interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

LGTM, thanks 🙏

@hacdias hacdias merged commit c178c51 into master May 8, 2023
@hacdias hacdias deleted the marcos-fx-foo branch May 8, 2023 14:11
hacdias added a commit that referenced this pull request May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

0.20.0-rc2: rc1 & rc2 fail to boot up with WebTransport in Addresses.Announce
2 participants