fix: await release in commit/rollback, set in-memory pool to max:1#1531
Draft
fix: await release in commit/rollback, set in-memory pool to max:1#1531
Conversation
Two latent bugs surfaced when enabling cds.env.features.pool='builtin':
1. DatabaseService.commit() and .rollback() called this.release() without
await. With generic-pool the synchronous internals masked this: the
connection was returned before any concurrent acquire could observe the
pool state. The builtin pool's release() is async (Promise-returning),
so without await the connection is not returned before the caller
continues, leading to connection leaks and stale-connection errors.
2. SQLiteService with ':memory:' databases: each sqlite(':memory:') call
creates a completely independent, isolated in-memory database. With a
pool max > 1, concurrent requests could acquire different connections,
each pointing to a separate empty database with no schema, causing
'no such table' errors. Capping the pool at max:1 for :memory:
databases serializes access through the single shared connection.
This is backward-compatible: generic-pool's synchronous release()
already serialized access effectively, and user config can still
override via options.pool.
Both fixes are backward-compatible with generic-pool.
Contributor
|
The max: 1 setting should not be needed since the setting should be applied through the package.json config. |
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.
Summary
This PR fixes two latent bugs in
DatabaseServicethat are masked when usinggeneric-poolbut surface when usingcds.env.features.pool = 'builtin'.The goal of the builtin pool is to be a drop-in replacement for
generic-pool. These fixes are needed to achieve that — and they are also correct standalone improvements, since both were pre-existing issues that happened not to matter with the old pool's synchronous internals.Fix 1:
await this.release()incommit()androllback()Root cause
DatabaseService.commit()androllback()calledthis.release()withoutawait:With
generic-pool,pool.release()performs all its internal bookkeeping synchronously (it only returns a Promise for API compatibility). So even withoutawait, the connection was returned to the pool before any other microtask could observe the pool state — the bug was silently harmless.The builtin pool's
release()is genuinely async: it enqueues the returning connection and resolves pending acquire calls asynchronously. Withoutawait, the caller continues before the connection is returned, leading to connection leaks and — when all pool slots are taken — acquire timeouts or stale-connection errors.Fix
Backward compatibility
generic-pool'srelease()also returns a Promise. Awaiting it was always valid — it just wasn't necessary before. This change is fully backward-compatible.Fix 2: cap
:memory:SQLite pool tomax: 1Root cause
Each call to
new sqlite(':memory:')(bothbetter-sqlite3and Node.js builtinsqlite) creates a completely independent, isolated in-memory database. There is no shared:memory:that multiple connections attach to.With a pool
max > 1, concurrent requests can acquire different connections, each pointing to a separate empty database with no schema, causingno such tableerrors.Note: SQLite does offer
file::memory:?cache=sharedfor shared in-memory databases, but neither driver uses it.Fix
Setting
max: 1for:memory:databases serializes all access through the single shared connection, which has the schema applied to it.User configuration (via
options.pool) still takes precedence through the spread, so users who know what they're doing can override.Backward compatibility
With
generic-pool's synchronousrelease(), increasingmaxabove 1 for:memory:made no observable behavioral difference — the connection was returned synchronously before any queued acquire could run, so requests were already effectively serialized. This fix preserves that behavior explicitly.Why making
builtin pool.release()synchronous is not the right fixAn alternative approach — making the builtin pool's
release()synchronous to matchgeneric-pool's internals — was investigated and ruled out:DatabaseService.release()is itselfasync, so even a synchronouspool.release()call would still yield to the event loop before the connection is returned (due to theasyncfunction's implicit Promise wrapping).DatabaseService.release()synchronous too would break thedbc.destroypattern used in some subclasses.The correct fix is to properly
awaitasync operations — which is what this PR does.Behavioral changes for users switching to the builtin pool
awaitonrelease()in user subclasses ofDatabaseServicewould now surface as bugs. This is a pre-existing latent issue, now exposed.acquireTimeoutMillis: null(no timeout), whereasgeneric-poolhas a default timeout. Misconfigured services that never release connections would silently queue forever instead of timing out. This can be configured viacds.env.requires.<db>.pool.acquireTimeoutMillis.Testing
The full
tests/_runtimesuite was run withcds.env.features.pool = 'builtin'after these fixes. All pre-existing test failures (17, all in-x4and REST test variants) remained unchanged. Zero new failures were introduced.