Repository navigation
fix(kad-dht): apply the timeout signal when pinging old contacts - #3671
Merged
Merged
Conversation
pingOldContacts() composed an abort signal from the ping timeout, the shutdown controller and the caller's signal, then passed the job's own options to pingContact() instead of that signal, so the composed signal was only ever cleared, never used. The job's signal aborts only once every caller waiting on the job has aborted, and outbound ping has no timeout of its own, so a contact that accepts the ping stream and never sends a pong wedged the job forever. That also blocked the bucket's add, which awaits the ping generator, held a slot in pingOldContactQueue for the lifetime of the node, and left the ping stream open until the connection died. The test asserts the outcome rather than just that the add returned, because every way of not reaching the ping at all also resolves it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
pingOldContacts()builds the abort signal it needs and then does not use it.signalsis only ever.clear()ed in thefinally. Theoptionspassed instead are the queue job's own, whose signal aborts only once every caller waiting on that job has aborted, sopingOldContactTimeoutnever applies.Outbound ping has no timeout of its own either.
@libp2p/pingbounds the pong wait solely withraceSignal(finished.promise, options.signal), and theAbortSignal.timeoutin that package is insidehandlePing, the inbound handler. So a contact that accepts the ping stream and never sends a pong wedges the job for the lifetime of the node.Three consequences:
_add()awaitsfor await (const toEvict of this.ping(toPing, options)), so the routing table insert that triggered the ping never completes.pingOldContactQueueslots, andpingOldContactQueue.find(peerId)keeps returning it, so every later ping of that peer joins the wedged job instead of starting its own.Ping.ping()only aborts its stream when the signal fires, so the ping stream stays open until the underlying connection dies.Found on a DHT server node after 13 days of uptime, with 13 of the 20 slots held by pings that had never timed out.
With the signal applied, a timed out ping rejects inside
pingContact(), which returnsfalse, so the unresponsive contact is evicted and the replacement is added.The test stubs a contact whose ping only settles on abort, then asserts that the ping ran, the contact was evicted, the new peer took its place, and the queue slot was released. Asserting only that the insert returned would be vacuous, since every path that skips the ping also returns.
Notes & open questions
The
catchinpingOldContactsdoes not fire on a timeout, which looks odd beside this change. It cannot fire at all:pingContact()wraps its whole body and returnsfalsefor any error, soping_old_contact_erroris never incremented. Pre-existing, and I would rather remove the dead branch separately than widen this PR.Other things in the same area, left out deliberately:
PING_OLD_CONTACT_TIMEOUTandPING_NEW_CONTACT_TIMEOUT(both2000) are exported and never read, so the effective bound isAdaptiveTimeout's own 5s rising to 60s. Wiring them up would change behaviour.verifyNewContact()passessignalstojoin()andadd()but hands the queue's options topingContact(), the same shape fixed here, so its timeout reaches the ping only viaJob.onAbort. It also starts the clock beforequeue.add(), so queue wait counts against the ping timeout.network.tscallsgetTimeoutSignal(options)in two places, which records a caller abort as a timeout failure and inflates the adaptive timeout.Change checklist