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

add test to demo missing peer records after listen #941

Merged
merged 4 commits into from
May 20, 2020

Conversation

yusefnapora
Copy link
Contributor

This shows off what I think is causing #939

The problem is that we're starting our hosts without listen addrs, and apparently the peerstore isn't keeping our signed records since they don't have any addrs. Then, when we call .Listen, it takes ~ 5 seconds for the change to be detected and the new record to be generated.

cc @aarshkshah1992 @raulk @vyzo

@yusefnapora
Copy link
Contributor Author

The last commits register BasicHost as a network.Notifee so it can get informed when we start or stop listening on an address and call SignalAddressChanges, which causes a new signed record to be created and added to the peerstore right away instead of having to wait for the 5 second ticker to detect the change.

It's still a little racy - on my laptop I only have to sleep for a millisecond after listening before I can get the new record out of the peerstore, but that's not quite long enough on Travis, so I bumped it to 20ms in the test. Still better than five seconds though :)

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

We could get rid of that race by waiting, but that's likely more trouble than it's worth. Instead, we should just fix the panic.

@Stebalien Stebalien merged commit aa60461 into master May 20, 2020
@Stebalien Stebalien mentioned this pull request May 26, 2020
77 tasks
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