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

relaysvc: fix flaky TestReachabilityChangeEvent #2215

Merged
merged 1 commit into from
Mar 24, 2023

Conversation

sukunrt
Copy link
Member

@sukunrt sukunrt commented Mar 24, 2023

fixes #2213

@sukunrt
Copy link
Member Author

sukunrt commented Mar 24, 2023

this line is expensive: https://github.com/libp2p/go-libp2p/blob/master/p2p/protocol/circuitv2/relay/relay.go#L629

But we can run gc separately in the Close call itself.

should we add this file to the repo? this was helpful
https://github.com/libp2p/go-libp2p/blob/e86584ffb2c17f05a494334acb7cef99b048cc00/docs/flaky-tests.md

@sukunrt sukunrt changed the title relaysvc: flaky TestReachabilityChangeEvent relaysvc: fix flaky TestReachabilityChangeEvent Mar 24, 2023
@MarcoPolo
Copy link
Collaborator

this line is expensive: https://github.com/libp2p/go-libp2p/blob/master/p2p/protocol/circuitv2/relay/relay.go#L629

Yeah, that's expected. It should be only needed once per process start though.

Copy link
Collaborator

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

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

Looks good

@MarcoPolo
Copy link
Collaborator

I'm guessing you removed the waitgroup since the gc method is safe to run multiple times, and you run the gc once more during close which effectively waits for the background goroutine to finish. Is that correct?

@sukunrt
Copy link
Member Author

sukunrt commented Mar 24, 2023

I'm guessing you removed the waitgroup since the gc method is safe to run multiple times, and you run the gc once more during close which effectively waits for the background goroutine to finish. Is that correct?

Yes. We want to run the gc once before closing to remove all the tagged connections.

The source of flakiness in the test is that the background go routine hasn't finished asnutil.Store.Init by the time we've called Close which leads to a timeout here
On calling Close it is waiting for the background goroutine to end.

It is fine to return before the background goroutine has finished as we've removed this relay out of the rest of the system by removing the streamhandlers and network.Notifee in Close and untagged all connections by running the gc once.

@MarcoPolo MarcoPolo merged commit 6cab5e8 into libp2p:master Mar 24, 2023
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.

relaysvc: flaky TestReachabilityChangeEvent
2 participants