feat: opt-in replacement for generic-pool#815
Conversation
| if (err.status == 404 || err.status == 429) | ||
| return new Promise(function (_, reject) { | ||
| if (err.status == 404 || err.status == 429) { | ||
| if (cds.requires.db?.pool?.builtin) throw err |
There was a problem hiding this comment.
Should that eventually be an internal server error? If yes, we have to change err.status
There was a problem hiding this comment.
With cap/cds/pull/5424 and this PR getting this error:
[error] - 500 - Error: Pool failed connecting to 't1'
at create (/Users/D061687/Desktop/dev/cds-dbs/hana/lib/HANAService.js:73:23)
at async /Users/D061687/Desktop/dev/cds-dbs/db-service/lib/common/generic-pool.js:182:45 {
'@Common.numericSeverity': 4,
code: '',
cause: Error: Tenant 't1' does not exist in Service Manager
at Object.get (/Users/D061687/Desktop/dev/cds-mtxs/srv/plugins/hana/srv-mgr.js:107:25)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
at async create (/Users/D061687/Desktop/dev/cds-dbs/hana/lib/HANAService.js:63:15)
at async /Users/D061687/Desktop/dev/cds-dbs/db-service/lib/common/generic-pool.js:182:45 {
status: 404
}
}
- 500 error in the client, generic "cannot connect" error message
- 404 error in the
causeproperty
→ should be what we want, right?
|
"Cause" chain I got in my manual tests: |
|
|
||
| async drain() { | ||
| this._draining = true | ||
| if (this._queue.length > 0) await this._queue.at(-1).promise |
There was a problem hiding this comment.
If the pool is being drained as preparation for it being destroyed it would make sense to reject every request in the queue. Rather then delaying the inevitable.
There was a problem hiding this comment.
Good point, should be fixed with e783311 where we now reject all resource requests immediately. Change looks bigger than it is as I added some Intellisense optimizations to help VS Code, as request.state access is not limited locally to acquire any more.
| await Promise.all(Array.from(this._creates)) | ||
| await Promise.all(Array.from(this._available).map(resource => this.#destroy(resource))) |
There was a problem hiding this comment.
Promise.all will reject when any of the provided promises reject. I don't think that clear should throw errors.
There was a problem hiding this comment.
Using Promise.allSettled now. Factory creation errors in #destroy are also logged here:
cds-dbs/db-service/lib/common/generic-pool.js
Line 257 in e783311
| this._creates.add(_create) | ||
| _create.finally(() => { | ||
| this._creates.delete(_create) | ||
| this.#dispense() |
There was a problem hiding this comment.
#createResource calls #dispense in finally and this finally handlers calls #dispense not really a problem, but calling it twice at the same event looks like double work.
There was a problem hiding this comment.
Avoided calling #dispense twice with 2cb41da.
|
@swaldmann @BobdenOs I propose to merge and further refine on the go. |
johannes-vogel
dismissed
danjoa’s stale review
questions are addressed, further refinement will be done on the go
🤖 I have created a release *beep* *boop* --- <details><summary>db-service: 2.1.0</summary> ## [2.1.0](db-service-v2.0.1...db-service-v2.1.0) (2025-06-04) ### Added * opt-in replacement for `generic-pool` ([#815](#815)) ([32c08f4](32c08f4)) </details> <details><summary>postgres: 2.0.2</summary> ## [2.0.2](postgres-v2.0.1...postgres-v2.0.2) (2025-06-04) ### Fixed * Allow raw streams to have empty results ([#1224](#1224)) ([0a59e69](0a59e69)) </details> <details><summary>hana: 2.1.0</summary> ## [2.1.0](hana-v2.0.1...hana-v2.1.0) (2025-06-04) ### Added * opt-in replacement for `generic-pool` ([#815](#815)) ([32c08f4](32c08f4)) ### Fixed * hana skip hextobin conversion for "select x from ..." input ([#1215](#1215)) ([e6d3d1b](e6d3d1b)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: Johannes Vogel <31311694+johannes-vogel@users.noreply.github.com>
5 participants
Drop-in replacement for
generic-pool.For now, we'll keep this opt-in – activated via
cds.requires.db.pool.builtin = true.