Use FinalizationRegistry for Dexie.connections and enforce maxConnections#2254
Use FinalizationRegistry for Dexie.connections and enforce maxConnections#2254dfahlander merged 7 commits intodexie:masterfrom
Conversation
…nections Historically, Dexie tracked Dexie instances in a global 'connections' array to emulate 'versionchange' events and support BFCache. However, if developers forget to call close() on a Dexie instance, it remains retained in this global array indefinitely, preventing garbage collection. This change replaces the global 'connections' array with a weak tracking mechanism using WeakRef and FinalizationRegistry. This allows Dexie instances that are no longer referenced by external code to be automatically garbage collected, even if not explicitly closed. Chromium developers have observed an egregious number of connections accumulating in real-world cases prior to this fix, leading to significant memory bloat in the browser process (hundreds of MiB). Additionally, this change introduces a 'maxConnections' limit (defaulting to 1000) to prevent unbounded growth of the number of connections if code unknowingly keeps references to a large number of instances. db.open() will now throw a MaxConnectionsReachedError if this limit is exceeded. These changes significantly improve the memory efficiency of Dexie by default while maintaining backward compatibility and providing robustness against connection leaks.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors global connection tracking into a dedicated connections manager module, adds DEFAULT_MAX_CONNECTIONS, exposes a Dexie option Changes
sequenceDiagram
participant Client
participant Dexie
participant ConnMgr as Connections Manager
participant FinReg as FinalizationRegistry
Client->>Dexie: new Dexie({maxConnections: N})
Dexie->>Dexie: store maxConnections in _options
Client->>Dexie: db.open()
Dexie->>ConnMgr: connections.add(db)
alt under limit
ConnMgr->>ConnMgr: create WeakRef(db), register with FinReg
ConnMgr-->>Dexie: added
else at limit
ConnMgr->>ConnMgr: evict oldest entry, unregister it
ConnMgr->>ConnMgr: create WeakRef(db), register with FinReg
ConnMgr-->>Dexie: added after eviction
end
Client->>Dexie: db.close()
Dexie->>ConnMgr: connections.remove(this)
ConnMgr-->>Dexie: removed
Note over FinReg,ConnMgr: When GC occurs, FinalizationRegistry triggers cleanup
FinReg->>ConnMgr: cleanup callback
ConnMgr->>ConnMgr: remove dead WeakRef
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
As discussed on this other pull request: #2250 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/classes/dexie/dexie-open.ts (1)
114-116: Guard invalidDexie.maxConnectionsvalues before enforcing the cap.Line 114 currently trusts the value. If set to
NaN, the cap is effectively disabled; if negative, all opens fail. Add a small sanity check before comparison.Proposed hardening
- if (connections.length >= Dexie.maxConnections) { + const maxConnections = Dexie.maxConnections; + if (Number.isNaN(maxConnections) || maxConnections < 0) { + throw new exceptions.InvalidArgument("Dexie.maxConnections must be >= 0 or Infinity"); + } + if (connections.length >= maxConnections) { throw new exceptions.MaxConnectionsReached(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/classes/dexie/dexie-open.ts` around lines 114 - 116, The current check trusts Dexie.maxConnections and can misbehave for NaN or negative values; before comparing connections.length to Dexie.maxConnections (the code that throws exceptions.MaxConnectionsReached), validate and normalize Dexie.maxConnections to a safe, finite non-negative integer (e.g., coerce NaN to a default like Infinity or a configured cap, clamp negatives to 0, and use Math.floor for non-integers) and then perform the existing comparison so the cap is enforced predictably.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/helpers/connection-tracker.ts`:
- Around line 91-99: The ConnectionsList type currently claims full array
semantics (Dexie[] & { delete(db: Dexie): void }) but ConnectionTracker only
implements a subset (push, delete, length, filter, forEach, map,
Symbol.iterator, toArray), so update the type to accurately reflect the
implemented API: replace the Dexie[] intersection with a narrow interface
listing only the actual methods and properties (e.g., push(item: Dexie): void;
delete(db: Dexie): void; readonly length: number; filter(...): Dexie[];
forEach(...): void; map(...): any; [Symbol.iterator](): Iterator<Dexie>;
toArray(): Dexie[]), and ensure createConnectionsList still returns that new
ConnectionsList type and ConnectionTracker implements it.
---
Nitpick comments:
In `@src/classes/dexie/dexie-open.ts`:
- Around line 114-116: The current check trusts Dexie.maxConnections and can
misbehave for NaN or negative values; before comparing connections.length to
Dexie.maxConnections (the code that throws exceptions.MaxConnectionsReached),
validate and normalize Dexie.maxConnections to a safe, finite non-negative
integer (e.g., coerce NaN to a default like Infinity or a configured cap, clamp
negatives to 0, and use Math.floor for non-integers) and then perform the
existing comparison so the cap is enforced predictably.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/classes/dexie/dexie-open.tssrc/classes/dexie/dexie.tssrc/errors/errors.jssrc/globals/constants.tssrc/helpers/connection-tracker.tssrc/index.tssrc/public/types/dexie-constructor.d.tssrc/public/types/errors.d.tssrc/support-bfcache.tstest/tests-all.jstest/tests-max-connections.js
| export type ConnectionsList = Dexie[] & { | ||
| delete(db: Dexie): void; | ||
| }; | ||
|
|
||
| export function createConnectionsList(): ConnectionsList { | ||
| const hasWeakSupport = typeof WeakRef !== 'undefined' && typeof Set !== 'undefined' && typeof FinalizationRegistry !== 'undefined'; | ||
| if (hasWeakSupport) { | ||
| return new ConnectionTracker() as any as ConnectionsList; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the ConnectionTracker implementation
cat -n src/helpers/connection-tracker.ts | head -150Repository: dexie/Dexie.js
Length of output: 3731
🏁 Script executed:
# Let's check what methods are actually implemented on ConnectionTracker
rg -nP 'class ConnectionTracker|^\s+(push|delete|filter|forEach|map|slice|find|some|reduce|\[Symbol\.iterator\]|get length)\s*\(' src/helpers/connection-tracker.ts -A 2Repository: dexie/Dexie.js
Length of output: 994
🏁 Script executed:
# Search for all usages of 'connections' variable/parameter with method calls
rg -nP 'connections\.(push|delete|filter|forEach|map|slice|find|some|reduce|length)' --type ts --type tsxRepository: dexie/Dexie.js
Length of output: 85
🏁 Script executed:
# Check all files that import or use ConnectionsList
rg -l 'ConnectionsList|createConnectionsList' --type ts --type tsxRepository: dexie/Dexie.js
Length of output: 85
🏁 Script executed:
# Search for connections usages without specific file type
rg 'connections\.(push|delete|filter|forEach|map|slice|find|some|reduce|indexOf|splice|length)' -nRepository: dexie/Dexie.js
Length of output: 1097
🏁 Script executed:
# Find files that import or use ConnectionsList or createConnectionsList
rg 'ConnectionsList|createConnectionsList' -lRepository: dexie/Dexie.js
Length of output: 116
🏁 Script executed:
# Check for any array method calls on connections variable
rg 'connections\.\w+\s*\(' -n | head -30Repository: dexie/Dexie.js
Length of output: 184
🏁 Script executed:
# Look for usage patterns to understand how connections is used
rg 'new ConnectionTracker\(\)|createConnectionsList\(\)' -B 3 -A 5Repository: dexie/Dexie.js
Length of output: 1602
ConnectionsList type advertises full array semantics but implementation only provides a subset.
The type definition Dexie[] & { delete(db: Dexie): void } (line 91) promises full array compatibility, but ConnectionTracker implements only push, delete, length, filter, forEach, map, Symbol.iterator, and toArray. It lacks slice, find, some, reduce, indexOf, splice, and other standard array methods.
While current in-repo usage only calls the implemented methods (push, delete, length), the type contract overpromises. Future code could call unimplemented array methods and fail at runtime despite passing TypeScript checks.
Narrow the type definition to match the actual implementation:
Suggested type refactor
-export type ConnectionsList = Dexie[] & {
- delete(db: Dexie): void;
-};
+export interface ConnectionsList extends Iterable<Dexie> {
+ readonly length: number;
+ push(db: Dexie): number;
+ delete(db: Dexie): void;
+ filter(callbackfn: (value: Dexie, index: number, array: Dexie[]) => boolean, thisArg?: any): Dexie[];
+ forEach(callbackfn: (value: Dexie, index: number, array: Dexie[]) => void, thisArg?: any): void;
+ map<U>(callbackfn: (value: Dexie, index: number, array: Dexie[]) => U, thisArg?: any): U[];
+ toArray(): Dexie[];
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type ConnectionsList = Dexie[] & { | |
| delete(db: Dexie): void; | |
| }; | |
| export function createConnectionsList(): ConnectionsList { | |
| const hasWeakSupport = typeof WeakRef !== 'undefined' && typeof Set !== 'undefined' && typeof FinalizationRegistry !== 'undefined'; | |
| if (hasWeakSupport) { | |
| return new ConnectionTracker() as any as ConnectionsList; | |
| } | |
| export interface ConnectionsList extends Iterable<Dexie> { | |
| readonly length: number; | |
| push(db: Dexie): number; | |
| delete(db: Dexie): void; | |
| filter(callbackfn: (value: Dexie, index: number, array: Dexie[]) => boolean, thisArg?: any): Dexie[]; | |
| forEach(callbackfn: (value: Dexie, index: number, array: Dexie[]) => void, thisArg?: any): void; | |
| map<U>(callbackfn: (value: Dexie, index: number, array: Dexie[]) => U, thisArg?: any): U[]; | |
| toArray(): Dexie[]; | |
| } | |
| export function createConnectionsList(): ConnectionsList { | |
| const hasWeakSupport = typeof WeakRef !== 'undefined' && typeof Set !== 'undefined' && typeof FinalizationRegistry !== 'undefined'; | |
| if (hasWeakSupport) { | |
| return new ConnectionTracker() as any as ConnectionsList; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/helpers/connection-tracker.ts` around lines 91 - 99, The ConnectionsList
type currently claims full array semantics (Dexie[] & { delete(db: Dexie): void
}) but ConnectionTracker only implements a subset (push, delete, length, filter,
forEach, map, Symbol.iterator, toArray), so update the type to accurately
reflect the implemented API: replace the Dexie[] intersection with a narrow
interface listing only the actual methods and properties (e.g., push(item:
Dexie): void; delete(db: Dexie): void; readonly length: number; filter(...):
Dexie[]; forEach(...): void; map(...): any; [Symbol.iterator]():
Iterator<Dexie>; toArray(): Dexie[]), and ensure createConnectionsList still
returns that new ConnectionsList type and ConnectionTracker implements it.
|
Re "ConnectionsList type advertises full array semantics but implementation only provides a subset" |
simpler WeakRef-based module (src/globals/connections.ts). Connections are now tracked via a Set<WeakRef<Dexie>> with a FinalizationRegistry for automatic cleanup when Dexie instances are garbage collected. Key changes: - Move maxConnections from a static property (Dexie.maxConnections) to a per-instance option in DexieOptions, defaulting to 1000 - Replace ConnectionTracker class with standalone addConnection(), removeConnection(), and getConnectionsArray() functions - Remove MaxConnectionsReached error — when the limit is exceeded, the oldest connection is evicted instead of throwing - Expose Dexie.connections as a getter that returns a snapshot array - Delete src/helpers/connection-tracker.ts
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/classes/dexie/dexie.ts (1)
99-109:⚠️ Potential issue | 🟠 MajorNormalize
maxConnectionsat construction time.Line 107 currently trusts user input. Non-finite or non-positive values can silently break tracking behavior. Clamp/validate once when building
this._options.💡 Proposed fix
constructor(name: string, options?: DexieOptions) { const deps = (Dexie as any as DexieConstructor).dependencies; + const inputMax = options?.maxConnections; + const normalizedMaxConnections = + Number.isInteger(inputMax) && (inputMax as number) > 0 + ? (inputMax as number) + : DEFAULT_MAX_CONNECTIONS; + this._options = options = { // Default Options addons: (Dexie as any as DexieConstructor).addons, // Pick statically registered addons by default autoOpen: true, // Don't require db.open() explicitely. // Default DOM dependency implementations from static prop. indexedDB: deps.indexedDB, // Backend IndexedDB api. Default to browser env. IDBKeyRange: deps.IDBKeyRange, // Backend IDBKeyRange api. Default to browser env. cache: 'cloned', // Default to cloned for backward compatibility. For best performance and least memory consumption use 'immutable'. - maxConnections: DEFAULT_MAX_CONNECTIONS, // Default max connections + maxConnections: normalizedMaxConnections, ...options };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/classes/dexie/dexie.ts` around lines 99 - 109, The constructor currently assigns user-supplied maxConnections directly into this._options; validate and normalize it when building this._options by checking the incoming options.maxConnections (or options after spread) and replacing it with a safe value if invalid—e.g., if not a finite number or <= 0, set to DEFAULT_MAX_CONNECTIONS; if fractional, coerce via Math.floor and ensure at least 1. Update the this._options assignment (the block that sets addons, autoOpen, indexedDB, IDBKeyRange, cache, maxConnections) so maxConnections is computed from the incoming options and normalized before being stored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/globals/connections.ts`:
- Around line 9-13: getConnectionsArray currently returns a new frozen snapshot
array (mapping _refs -> deref) which breaks callers that cache Dexie.connections
expecting a live-updating list; change getConnectionsArray to return a live view
instead of a snapshot by returning a Proxy (or a custom iterable object) that on
property access/indexing/iteration always reads current _refs and dereferences
them (use _refs, ref.deref(), and handle undefined filtering) so length, numeric
indices, iteration, and array-like access reflect live updates rather than a
one-time array snapshot.
- Around line 19-24: Eviction currently only removes the reference from _refs
and calls _registry.unregister(oldestRef) but doesn't close the actual DB
connection; update the eviction code to first obtain the underlying connection
instance associated with oldestRef (via whatever association exists in your
module — e.g., a map from ref→db or a lookup on _registry), call its
close/destroy/cleanup method (await it if it returns a promise) and handle any
errors, and only after successful close remove the ref from _refs and call
_registry.unregister(oldestRef); ensure you reference _refs,
_registry.unregister, oldestRef and db._options.maxConnections when making the
change.
- Around line 27-33: The removeConnection function dereferences db._novip even
when db may be undefined; add a guard at the start of removeConnection (or use
optional chaining) to handle undefined db safely—e.g., return early if db is
undefined or change comparisons to use db?._novip so ref.deref() is never
compared to an invalid property; update the check inside removeConnection (the
ref.deref() === db._novip comparison) to use the guarded value to avoid runtime
exceptions.
- Around line 3-6: Guard creation and usage of WeakRef/FinalizationRegistry by
checking runtime support (typeof WeakRef !== "undefined" and typeof
FinalizationRegistry !== "undefined") before instantiating _refs and _registry
so module import on ES5 targets won't throw; create fallbacks (e.g., no-op
registry and Set) when not available and only call FinalizationRegistry methods
when present. Also update removeConnection to null-check the db parameter (or
db._novip) before accessing db._novip to avoid a TypeError when removeConnection
is called with undefined.
In `@src/globals/global.ts`:
- Line 1: Remove the unused top-level import of Dexie from this module to break
the circular initialization (delete the line "import { Dexie } from
\"../classes/dexie\""); ensure no code in this file references Dexie at module
scope—if other modules need Dexie during runtime, import it lazily where used
(or use an import type) instead so _global is not accessed during module
initialization and the circular dependency via src/functions/utils.ts ->
src/globals/global.ts is eliminated.
---
Outside diff comments:
In `@src/classes/dexie/dexie.ts`:
- Around line 99-109: The constructor currently assigns user-supplied
maxConnections directly into this._options; validate and normalize it when
building this._options by checking the incoming options.maxConnections (or
options after spread) and replacing it with a safe value if invalid—e.g., if not
a finite number or <= 0, set to DEFAULT_MAX_CONNECTIONS; if fractional, coerce
via Math.floor and ensure at least 1. Update the this._options assignment (the
block that sets addons, autoOpen, indexedDB, IDBKeyRange, cache, maxConnections)
so maxConnections is computed from the incoming options and normalized before
being stored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7eb57b1c-3700-4aa1-b18f-bae6465ecafc
📒 Files selected for processing (9)
src/classes/dexie/dexie-open.tssrc/classes/dexie/dexie-static-props.tssrc/classes/dexie/dexie.tssrc/globals/connections.tssrc/globals/constants.tssrc/globals/global.tssrc/public/types/dexie-constructor.d.tssrc/support-bfcache.tstest/tests-max-connections.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/public/types/dexie-constructor.d.ts
- test/tests-max-connections.js
- src/globals/constants.ts
- src/classes/dexie/dexie-open.ts
- src/support-bfcache.ts
| export function getConnectionsArray(): Dexie[] { | ||
| return Array.from(_refs) | ||
| .map(ref => ref.deref()) | ||
| .filter((db): db is Dexie => db !== undefined); | ||
| } |
There was a problem hiding this comment.
getConnectionsArray() returns snapshots, not a live list (behavioral compatibility risk).
Line 9-13 creates a new array each call. Code that caches Dexie.connections will now observe stale state instead of live updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/globals/connections.ts` around lines 9 - 13, getConnectionsArray
currently returns a new frozen snapshot array (mapping _refs -> deref) which
breaks callers that cache Dexie.connections expecting a live-updating list;
change getConnectionsArray to return a live view instead of a snapshot by
returning a Proxy (or a custom iterable object) that on property
access/indexing/iteration always reads current _refs and dereferences them (use
_refs, ref.deref(), and handle undefined filtering) so length, numeric indices,
iteration, and array-like access reflect live updates rather than a one-time
array snapshot.
There was a problem hiding this comment.
Dexie.connections is an undocumented property and is lacking in public typings. It has never been meant as anything other than a readonly array. We're still considering removing it or decide to document it as official, but for now we're focusing on runtime compability since it's been there for a long time. However, we've decided not to over-complicate this workaround.
There was a problem hiding this comment.
@dfahlander, understood — since Dexie.connections is undocumented, lacks public typings, and has always been intended as a readonly snapshot rather than a live-updating collection, the snapshot approach in getConnectionsArray() is perfectly appropriate and adding a Proxy would be unnecessary complexity. I'll withdraw this concern.
✏️ Learnings added
Learnt from: dfahlander
Repo: dexie/Dexie.js PR: 2254
File: src/globals/connections.ts:9-13
Timestamp: 2026-03-04T16:56:43.369Z
Learning: In `src/globals/connections.ts` (Dexie.js), `Dexie.connections` is an undocumented, internal property with no public typings. It has always been intended as a readonly snapshot array, not a live-updating collection. Do not flag `getConnectionsArray()` returning a new snapshot array as a behavioral compatibility issue — a live Proxy or similar mechanism is explicitly considered over-engineering and rejected by the maintainer (dfahlander). The property may be removed or officially documented in the future, but for now snapshot semantics are intentional.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: dfahlander
Repo: dexie/Dexie.js PR: 2254
File: src/globals/connections.ts:19-24
Timestamp: 2026-03-04T16:45:05.681Z
Learning: In src/globals/connections.ts, treat silent eviction (untracking) of a connection without an explicit close as an acceptable trade-off. This aligns with the planned removal of Dexie.js's synthetic versionchange event and the non-critical nature of BFCache support in modern Chrome. Do not flag silent eviction as a bug; instead, document it as intentional behavior and ensure tests reflect this expectation.
| if (_refs.size > db._options.maxConnections) { | ||
| // Remove the oldest connection (the one that was added first) | ||
| const oldestRef = _refs.values().next().value; | ||
| _refs.delete(oldestRef); | ||
| _registry.unregister(oldestRef); | ||
| } |
There was a problem hiding this comment.
Eviction currently removes tracking only, not the underlying open connection.
Line 19-24 drops the oldest reference but leaves the actual DB instance open. This can hide active connections from mechanisms that rely on tracked connections (for example, synthetic versionchange dispatch in src/classes/dexie/dexie.ts Line 231-233).
💡 Proposed fix options
if (_refs.size > db._options.maxConnections) {
const oldestRef = _refs.values().next().value;
+ // Option A: enforce limit by failing fast (throw) before adding.
+ // Option B: close the evicted live connection before untracking.
_refs.delete(oldestRef);
_registry.unregister(oldestRef);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/globals/connections.ts` around lines 19 - 24, Eviction currently only
removes the reference from _refs and calls _registry.unregister(oldestRef) but
doesn't close the actual DB connection; update the eviction code to first obtain
the underlying connection instance associated with oldestRef (via whatever
association exists in your module — e.g., a map from ref→db or a lookup on
_registry), call its close/destroy/cleanup method (await it if it returns a
promise) and handle any errors, and only after successful close remove the ref
from _refs and call _registry.unregister(oldestRef); ensure you reference _refs,
_registry.unregister, oldestRef and db._options.maxConnections when making the
change.
There was a problem hiding this comment.
I'm aware of that, but the synthetic versionchange event will be gone soon because it was built for very old browsers. The bfcache support is not crucial either and would not break anything if missed. And it is already handled by modern chrome browsers.
There was a problem hiding this comment.
@dfahlander, understood — thanks for the context! Since the synthetic versionchange dispatch is slated for removal and bfcache is already handled natively by modern browsers, the silent eviction approach is a reasonable trade-off. I'll drop the concern.
✏️ Learnings added
Learnt from: dfahlander
Repo: dexie/Dexie.js PR: 2254
File: src/globals/connections.ts:19-24
Timestamp: 2026-03-04T16:45:05.681Z
Learning: In Dexie.js, the synthetic `versionchange` event dispatch (used in `src/classes/dexie/dexie.ts` around the versionchange workaround) is planned for removal as it was built for very old browsers. BFCache support (`src/support-bfcache.ts`) is also not critical since modern Chrome handles it natively. Therefore, silently evicting (untracking) a connection without explicitly closing it in `src/globals/connections.ts` is an acceptable trade-off and not a bug that needs fixing.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Thanks a lot for the PR. I adjusted it slightly to reduce code and handle maxConnections as a constructor arg + not throw. I hesitated about not throwing though, but considered it to be most non-intrusive. Dexie.connections is a hidden feature and the only "problem" would be that the connection was hidden from it, so I thought there would be no reason to throw... |
… old browsers that lacks FinalizationRegistry or WeakRef.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/globals/connections.ts`:
- Around line 22-33: The add function creates a WeakRef to db._novip but
registers db with the FinalizationRegistry, causing identity mismatch; update
add so the same object is used for both WeakRef and
FinalizationRegistry.register (use target = db._novip for register and also use
a consistent token when storing in _refs/_registry) and adjust unregister to
pass that same token (the stored reference/token) so the WeakRef target and the
FinalizationRegistry target match (ensure references added to _refs, the
register() call, and the unregister() call all reference db._novip or the same
stored token).
- Around line 58-63: The remove() fallback currently uses indexOf(db) which
fails when callers pass a VIP proxy; update remove() to detect and remove the
stored entry by comparing the stored item to db and to db._novip (mirroring the
WeakRef path), i.e. when iterating connections check for stored === db || stored
=== db._novip and splice that index; also ensure add() pushes db._novip (not the
proxy) so connections always contain the original instance, and keep references
to symbols remove(), add(), _close(), _novip and the connections array when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6053a13a-dd0f-437c-a477-fab23205ca0d
📒 Files selected for processing (5)
src/classes/dexie/dexie-open.tssrc/classes/dexie/dexie-static-props.tssrc/classes/dexie/dexie.tssrc/globals/connections.tssrc/support-bfcache.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/classes/dexie/dexie-open.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/globals/connections.ts (1)
55-57:⚠️ Potential issue | 🟠 MajorFallback path does not enforce
maxConnections.In the non-WeakRef branch,
add()only pushes intoconnectionsand never applies the cap. On runtimes without WeakRef/FinalizationRegistry, tracking can still grow unbounded.💡 Proposed fix
const add = (db: Dexie) => { connections.push(db._novip); + if (connections.length > db._options.maxConnections) { + connections.shift(); // evict oldest tracked connection + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/globals/connections.ts` around lines 55 - 57, The non-WeakRef path's add(db: Dexie) currently pushes db._novip into connections but never enforces maxConnections, allowing unbounded growth; update add (referencing add, connections, maxConnections, and db._novip) to apply the same cap logic used in the WeakRef branch: after pushing, trim the connections array down to maxConnections (remove oldest entries) and perform any necessary cleanup for removed entries (the same cleanup used in the WeakRef path) so the fallback path maintains the same connection limit behavior.
🧹 Nitpick comments (1)
src/globals/connections.ts (1)
54-54: Return a snapshot in fallbacktoArray()instead of the mutable backing array.Returning the backing array exposes internal state to external mutation and diverges from readonly snapshot intent.
💡 Proposed refactor
- const toArray = (): ReadonlyArray<Dexie> => connections; + const toArray = (): ReadonlyArray<Dexie> => connections.slice();Based on learnings:
Dexie.connectionsis intended to behave as a readonly snapshot array.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/globals/connections.ts` at line 54, The toArray function currently returns the mutable backing array "connections", exposing internal state; change toArray (the exported function named toArray) to return an immutable snapshot (e.g., a shallow copy of "connections") so callers receive a ReadonlyArray<Dexie> snapshot rather than the original array; update the return to produce a copy of "connections" (preserving type ReadonlyArray<Dexie>) to prevent external mutation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/globals/connections.ts`:
- Around line 7-19: The toArray() implementation currently caches dereferenced
Dexie instances in the module-scoped _array which creates strong references and
prevents garbage collection via WeakRef/FinalizationRegistry; remove the cached
_array entirely and have toArray() always build and return a fresh snapshot
array (e.g., Array.from(_refs).map(...).filter(...)) so no strong references are
held between calls, and also ensure the fallback path returns a shallow copy of
connections (not the live array) to preserve snapshot semantics; finally remove
any assignments that set _array = null in add() and remove() since the cache
will be gone.
---
Duplicate comments:
In `@src/globals/connections.ts`:
- Around line 55-57: The non-WeakRef path's add(db: Dexie) currently pushes
db._novip into connections but never enforces maxConnections, allowing unbounded
growth; update add (referencing add, connections, maxConnections, and db._novip)
to apply the same cap logic used in the WeakRef branch: after pushing, trim the
connections array down to maxConnections (remove oldest entries) and perform any
necessary cleanup for removed entries (the same cleanup used in the WeakRef
path) so the fallback path maintains the same connection limit behavior.
---
Nitpick comments:
In `@src/globals/connections.ts`:
- Line 54: The toArray function currently returns the mutable backing array
"connections", exposing internal state; change toArray (the exported function
named toArray) to return an immutable snapshot (e.g., a shallow copy of
"connections") so callers receive a ReadonlyArray<Dexie> snapshot rather than
the original array; update the return to produce a copy of "connections"
(preserving type ReadonlyArray<Dexie>) to prevent external mutation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91884ef6-6d21-4d6c-b07e-310daf30c6be
📒 Files selected for processing (1)
src/globals/connections.ts
Refactor connections manager to remove unnecessary array caching.
3 participants
Historically, Dexie tracked Dexie instances in a global 'connections' array to emulate 'versionchange' events and support BFCache. However, if developers forget to call close() on a Dexie instance, it remains retained in this global array indefinitely, preventing garbage collection.
This change replaces the global 'connections' array with a weak tracking mechanism using WeakRef and FinalizationRegistry. This allows Dexie instances that are no longer referenced by external code to be automatically garbage collected, even if not explicitly closed. Chromium developers have observed an egregious number of connections accumulating in real-world cases prior to this fix, leading to significant memory bloat in the browser process (hundreds of MiB).
Additionally, this change introduces a 'maxConnections' limit (defaulting to 1000) to prevent unbounded growth of the number of connections if code unknowingly keeps references to a large number of instances. db.open() will now throw a MaxConnectionsReachedError if this limit is exceeded.
These changes significantly improve the memory efficiency of Dexie by default while maintaining backward compatibility and providing robustness against connection leaks.
Summary by CodeRabbit
New Features
API Changes
Tests