feat(pool): enable reconnect by default, add disconnect/reconnect callbacks#534
Open
kai-familiar wants to merge 4 commits into
Open
feat(pool): enable reconnect by default, add disconnect/reconnect callbacks#534kai-familiar wants to merge 4 commits into
kai-familiar wants to merge 4 commits into
Conversation
…lbacks
- Change enableReconnect default from false to true in SimplePool
- Relays that disconnect during long-running subscriptions now
automatically reconnect with exponential backoff
- Existing resubscription logic in AbstractRelay already handles
re-sending REQ with updated since filters
- Opt out with enableReconnect: false
- Add onreconnect callback to AbstractRelay
- Fires when a relay successfully reconnects after disconnection
- Add onRelayDisconnect/onRelayReconnect callbacks to pool options
- Enables monitoring connection health in long-running clients
Fixes nbd-wtf#513
onclose is not called when enableReconnect is true; the relay goes straight to reconnection. The test was waiting for an event that never fires, causing a 20s timeout. This was a pre-existing failure on master.
Root causes found and fixed: 1. Pool tests set pingTimeout/pingFrequency AFTER ensureRelay() calls connect(), but the ping interval was already created with the default 29s frequency. Fix: restart the interval after changing settings. 2. Reconnect tests waited for onclose, but onclose is not called when enableReconnect is true — the relay goes straight to reconnection. Fix: wait for relay.connected === false instead. 3. Pool 'ping-pong timeout' test didn't set enableReconnect: false, which now defaults to true (changed in this PR). Fix: explicitly disable reconnect for the timeout-only test. All 4 previously-failing tests now pass in <500ms each.
Contributor
Author
|
CI update: format check now passes ✅. The only remaining test failure is As a bonus, this PR also fixes 4 pre-existing test failures on master:
Root causes: ping interval created with 29s default before test could override settings, and reconnect tests waiting for |
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.
Problem
Fixes #513 — Long-running
SimplePoolsubscriptions degrade silently as relays disconnect one by one. The pool waits for ALL relay subscriptions to close before firingonclose, so clients have no visibility into individual relay health.Root Cause
AbstractRelayalready has robust reconnection logic (enableReconnect, exponential backoff viaresubscribeBackoff, automatic re-subscription withsincefilter adjustment). However,SimplePooldefaultsenableReconnecttofalse, so this existing machinery is unused unless explicitly opted in.Changes
Default
enableReconnecttotrueinSimplePool(wasfalse)sincefilters to avoid duplicate eventsenableReconnect: falsefor backward compatibilityAdd
onreconnectcallback toAbstractRelayonclosefor full lifecycle visibilityAdd
onRelayDisconnect/onRelayReconnectcallbacks to pool optionsonRelayConnectionFailure/onRelayConnectionSuccessUsage
Breaking Change
enableReconnectnow defaults totrueinstead offalse. Existing code that relied on the pool NOT reconnecting should explicitly passenableReconnect: false. In practice, most long-running clients want reconnection, so this should be a net improvement.