Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

dial: limit error size #120

Closed
wants to merge 2 commits into from
Closed

dial: limit error size #120

wants to merge 2 commits into from

Conversation

Stebalien
Copy link
Member

Instead of storing every error, store at most 32 errors (plus a "too many errors" error).

Helps address #119

Note: libp2p/go-libp2p-kad-dht#328 actually fixes the issue but this PR helps reduce the impact of similar issues.

Instead of storing _every_ error, store at most 32 errors (plus a "too many errors" error).

Helps address #119
@ghost ghost assigned Stebalien Apr 24, 2019
@ghost ghost added the status/in-progress In progress label Apr 24, 2019
@Stebalien Stebalien requested a review from vyzo April 24, 2019 16:08
@@ -58,6 +61,9 @@ const ConcurrentFdDials = 160
// per peer
const DefaultPerPeerRateLimit = 8

// maxDialErrors is the maximum number of dial errors we record
const maxDialErrors = 32
Copy link
Contributor

Choose a reason for hiding this comment

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

this might even be too many; maybe 16 or 10?

@raulk
Copy link
Member

raulk commented Apr 24, 2019

For usability, it would be cool to:

  • include the value of maxDialErrors inside the message for ErrTooManyErrors, e.g. "too many errors (max tracked: 32)".
  • turn ErrTooManyErrors into a struct that counts how many more errors we found beyond our max; when we reach the threshold, we can retain a pointer to it and keep incrementing the counter for every additional error. That way we could, at least, tell the user how many more errors we saw but failed to retain vs. throwing our hands up in the air entirely.

Also, always include the context error.
@Stebalien
Copy link
Member Author

Alternative here: #120.

@Stebalien
Copy link
Member Author

(closing in favor of the alternative, we can reopen if necessary)

@Stebalien Stebalien closed this Apr 25, 2019
@ghost ghost removed the status/in-progress In progress label Apr 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants