Explicit Resource Management for pg pool client#3661
Explicit Resource Management for pg pool client#3661hyperair wants to merge 16 commits intobrianc:masterfrom
Conversation
bac8c73 to
7d69534
Compare
|
Hmm, looks like eslint needs to be upgraded before it'll parse the |
7d69534 to
0cb5f67
Compare
- rootDir defaults have changed, so we need to specify it manually now - baseUrl is no longer supported - types no longer loads everything in @types by default, so we have to specify that we want node types - Pin @types/node to 16.* because we support node16 and above
Fixes the following error:
% yarn build
yarn run v1.22.19
$ tsc --build
packages/pg-cloudflare/src/index.ts:156:29 - error TS2769: No overload matches this call.
The last overload gave the following error.
Argument of type 'ArrayBuffer | Uint8Array<ArrayBufferLike>' is not assignable to parameter of type 'WithImplicitCoercion<string> | { [Symbol.toPrimitive](hint: "string"): string; }'.
Type 'ArrayBuffer' is not assignable to type 'WithImplicitCoercion<string> | { [Symbol.toPrimitive](hint: "string"): string; }'.
156 const hex = Buffer.from(data).toString('hex')
~~~~
node_modules/@types/node/buffer.buffer.d.ts:83:13
83 from(
~~~~~
84 str:
~~~~~~~~~~~~~~~~~~~~
...
89 encoding?: BufferEncoding,
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
90 ): Buffer<ArrayBuffer>;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The last overload is declared here.
Found 1 error.
See microsoft/TypeScript#63447 for more info
Fixes the following typescript error:
node_modules/typescript/lib/lib.esnext.intl.d.ts:26:135 - error TS2552: Cannot find name 'DateTimeRangeFormatPart'. Did you mean 'DateTimeFormatPart'?
26 formatRangeToParts(startDate: FormattableTemporalObject | Date | number, endDate: FormattableTemporalObject | Date | number): DateTimeRangeFormatPart[];
`BufferReader.encoding` to `BufferEncoding` from `string` to match the new signature of `Buffer.toString`.
0cb5f67 to
24caf46
Compare
There was a problem hiding this comment.
Not sure about destroyOnDispose, i.e. whether its job is common enough to justify it and whether it’s the best API for the job. Would suggest initializing to false if kept. Note ; I recognize that it doesn’t satisfy the use case you mentioned. Maybe release the feature without that API initially, since it can be added easily?.release(true) continues to work for error cases given that double release is a no-op
Edit: Sorry, I don’t know where I got that from – we haven’t even done PooledClient yet.
|
|
||
| client.release = this._releaseOnce(client, idleListener) | ||
|
|
||
| if (Symbol.dispose) { |
There was a problem hiding this comment.
I think this would normally go on the prototype, but pg-pool’s approach is already a bit messy (it’s lacking a PooledClient object that’s specific to one acquisition operation) so it’s also not the end of the world if the patch goes in as-is.
There was a problem hiding this comment.
Agreed, and that's why I implemented it like this.
@types/pg has a PooledClient type with the release signature, and I intend to make a PR there to add the [Symbol.dispose] addition later.
Yeah I wasn't sure about the API either but I couldn't think of a better way to do it. Maybe a That said, unfortunately I don't think relying on double-release is workable either, because the second release, which will be called from within |
When `Symbol.dispose` is defined, define a disposer function that simply calls `this.release()`. This lets the `PoolClient` with the `using` syntax work with any downstream-overridden `release` functions. This makes `PoolClient` support Explicit Resource Management when the runtime supports it. Fixes: brianc#3515
Add a `destroyOnDispose` property on the client to signal that `[Symbol.dispose]()` should call `client.release(true)` instead of `client.release()`.
Co-authored-by: Charmander <[email protected]>
`connect` has two `n`s, and we don't like semicolons Co-authored-by: Charmander <[email protected]>
36ef0cf to
8c8d974
Compare
Add support for Explicit Resource Management for
pg.Poolclients.This is implemented using
[Symbol.dispose]instead of[Symbol.asyncDispose]becauseclient.release()is a synchronous function, not async.Additionally, add a second property
client.destroyOnDisposeso thatSymbol.disposesupports callingclient.release(true). This is useful for a usecase that changes connection parameters in a way that is troublesome to reset upon release to the pool, e.g.Fixes: #3515
Note: This PR has been rebased on top of #3662, so there are some irrelevant commits until that PR is merged.