-
Notifications
You must be signed in to change notification settings - Fork 37
fix: improve dial queue and parallel dials #315
Conversation
src/limit-dialer/index.js
Outdated
}) | ||
}) | ||
|
||
tryEach(tasks, (_, result) => { | ||
if (result && result.conn) { | ||
parallel(tasks, (_) => { |
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.
Actually, do you want to use async/race
for this instead so you don't have to wait for all the dials to finish before calling back?
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 will use each. race
will finish as soon as any of the dial callbacks finish, even if nothing is passed. So if we dial 5 addresses and the first errors, even if we just store an array of the errors as soon as we callback it ends, so we end up having a "failed" dial after the first one errors. Even if the other 4 were actually good.
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.
Okay, I did go with race for the aforementioned benefits. I just only call the callback if we have a value, or if everything errored.
} | ||
return callback(null, { cancel: true }) | ||
return callback(null) |
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.
What does removing these objects do?
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 believe these were left over from some old code, only valid connection results are being used now, so they were deleted. The intent is that it creates a cancel token that causes other successfully dialed connections to be destroyed. Now the token is created when we receive the first valid connection.
docs: fix spelling
Improved Dialer
This improves the dialer queue a bit by doing a few things:
Multidimensional queues
As dial requests are made they no longer go into a single, global queue, they are immediately added to the dial queue for that specific peer. This allows a successful queue, or one that errors, to immediately resolve the remaining dialing requests. Previously, a subsequent request would need to wait until its turn globally, which is slow, especially if it just needs to be aborted or a new stream needs to be created for a protocol.
Immediate dials for connected peers
If a peer is already connected, we immediately start its queue if it's not already running. The max parallel queues aren't affected by this. This ensures that we have a steady stream of communication with our connected peers, which is critical to maintaining a healthy network.
Configurable dial timeout
The dial timeout was also not configurable. This has been corrected and added to the readme.