-
Notifications
You must be signed in to change notification settings - Fork 445
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: do not auto-dial peers in the dial queue #1740
fix: do not auto-dial peers in the dial queue #1740
Conversation
Similar to #1730, filter auto-dial peers by peers already in the dial queue. The dial queue will join any pre-existing dial attempts for the selected peers, but it just creates unnecessary work and adds noise to the logs and filtering them out beforehand is cheap so just do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Looks good overall just some recommendations
@@ -103,6 +103,12 @@ export class AutoDial implements Startable { | |||
|
|||
const connections = this.connectionManager.getConnectionsMap() | |||
const numConnections = connections.size | |||
const dialQueue = new PeerSet( | |||
// @ts-expect-error boolean filter removes falsy peer IDs | |||
this.connectionManager.getDialQueue() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type casting should work here and remove the need for ugly //@ts-expect-error
s
this.connectionManager.getDialQueue() | |
const dialQueue = new PeerSet( | |
this.connectionManager.getDialQueue() | |
.map(queue => queue.peerId) | |
.filter(Boolean) as PeerId[] | |
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing about the ugly @ts-expect-error
is that should typescript ever be smart enough to work out what .filter(Boolean)
does, it'll then become an error itself letting you know that the types are correctly inferred now whereas the as
cast will smother the error forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point but wouldn't a PeerId[]
type be expected here? I think the type error being returned by .filter(Boolean)
is legitimate because all values could be falsey, which in that case it would be more appropriate to have an empty array of PeerId
passed to new PeerSet
as opposed to undefined
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return type from the boolean filter is PeerId[]
- if all values in the list are falsey before filtering, an empty array is returned:
[false, 0, null, undefined].filter(Boolean)
// []
This is a bug in typescript. The issue reporting the bug is here: microsoft/TypeScript#16655 - it was fixed by microsoft/TypeScript#29955 and then reverted by microsoft/TypeScript#16655 (comment) so remains unfixed.
Similar to #1730, filter auto-dial peers by peers already in the dial queue.
The dial queue will join any pre-existing dial attempts for the selected peers, but it just creates unnecessary work and adds noise to the logs and filtering them out beforehand is cheap so just do that.