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

fix: do not auto-dial peers in the dial queue #1740

Merged
merged 3 commits into from May 9, 2023

Conversation

achingbrain
Copy link
Member

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.

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.
Copy link
Member

@maschad maschad left a 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()
Copy link
Member

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

Suggested change
this.connectionManager.getDialQueue()
const dialQueue = new PeerSet(
this.connectionManager.getDialQueue()
.map(queue => queue.peerId)
.filter(Boolean) as PeerId[]
)

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

test/connection-manager/auto-dial.spec.ts Outdated Show resolved Hide resolved
@achingbrain achingbrain merged commit 124ca8a into master May 9, 2023
25 checks passed
@achingbrain achingbrain deleted the fix/do-not-auto-dial-peers-in-the-dial-queue branch May 9, 2023 08:30
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.

None yet

2 participants