-
Type:
Bug
-
Resolution: Unresolved
-
Priority:
Minor - P4
-
None
-
Affects Version/s: None
-
Component/s: CSOT
Use Case
As a... (who is this for)
I want... (what is the desired change)
So that... (why is the change desired)
User Experience
Timeout extends Promise<never> and is used typically in conjunction with Promise.race - Promise.race([operation, timeout]) for CSOT, Server selection, and socket I/O. When .clear() is called, it stops the underlying timer but never settles the Promise.
// Nothing about the underlying Promise here - it's still "pending" /** * Clears the underlying timeout. This method is idempotent */ clear(): void { clearTimeout(this.id); this.id = undefined; this.timedOut = false; this.cleared = true; }
When the operation (from the example above) wins the race and the Timeout is cleared, unsettled Promise remains in memory.
This was identified during NODE-7142 (Client backpressure) where a bug in the retry loop caused Timeouts to be creating in a loop. Those unsettled Promises chained faster than GC could collect them, and this lead to OOM crashes. The cause (infinite retry loop) was fixed, but the unsettled Promise pattern remains.
We can reject this promise when it should be cleared and swallow error to avoid any side-effect.
/** Create a new timeout that expires in `duration` ms */ private constructor( executor: Executor = () => null, options?: { duration: number; unref?: true; rejection?: Error } ) { ... let reject!: Reject; super((_, promiseReject) => { reject = promiseReject; executor(noop, promiseReject); }); this.reject = reject; // keep the reference in instance level ... } /** * Clears the underlying timeout. This method is idempotent */ clear(): void { if (this.cleared) return; // keep this method idempotent clearTimeout(this.id); this.id = undefined; this.timedOut = false; this.cleared = true; this.then(undefined, squashError); this.reject(); // don't need to provide anything, just settle the promise }
Verification needed that no existing paths depend on cleared Timeout staying pending (very unlikely).
This is not leading to any significant problems right now as there are normally not much Timeouts (one per session-scoped TimeoutContext).
Dependencies
- upstream and/or downstream requirements and timelines to bear in mind
Risks/Unknowns
- What could go wrong while implementing this change? (e.g., performance, inadvertent behavioral changes in adjacent functionality, existing tech debt, etc)
- Is there an opportunity for better cross-driver alignment or testing in this area?
- Is there an opportunity to improve existing documentation on this subject?
Acceptance Criteria
Implementation Requirements
- functional reqs, potential snafus to avoid, performance targets, etc
Testing Requirements
- unit test, spec test sync, etc
Documentation Requirements
- DOCSP ticket, API docs, etc
Follow Up Requirements
- additional tickets to file, required releases, etc
- if node behavior differs/will differ from other drivers, confirm with dbx devs what standard to aim for and what plan, if any, exists to reconcile the diverging behavior moving forward