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

Don't stop browsing after ~15min #13

Merged
merged 1 commit into from
Sep 22, 2021
Merged

Conversation

betamos
Copy link

@betamos betamos commented Sep 8, 2021

I was surprised to discover that browse terminates, closing the entries channel, after ~15 minutes. Clearly, there was logic present handling the stop/timeout case in the original code. A simple repro can be achieved using the example:

time go run examples/resolv/client.go -wait 3600
2021/09/08 02:19:40 Failed to browse: periodicQuery: abort due to timeout
exit status 1
go run examples/resolv/client.go -wait 3600  0,39s user 0,17s system 0% cpu 14:45,19 total

I changed it the way I think is most intuitive, which is not timeout at all. My use case is a long-running application and while I could certainly browse multiple times, I don't see why. It looks like legacy from the original repo. Timeouts can be handled the usual way, using context.WithTimeout, if required.

Previously, browse would complete after roughly 15min,
the default MaxElapsedTime in the exponential backoff algorithm.
Copy link

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Thanks for catching this! This is a quite error-prone API this the backoff package is offering there, setting a default value of 15 minutes unless manually reset.

I think we might be able to get rid of this library altogether. Calculating an exponential backoff is really not that hard, even if you want to add some jitter. Will do so in a follow-up PR.

@marten-seemann marten-seemann merged commit 9dfbd03 into libp2p:master Sep 22, 2021
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 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