Do not return broken or closed clients to the pool #2081
Closed
Conversation
This file contains 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
* Drop support for EOL versions of node * Re-add testing for [email protected] * Revert changes to .travis.yml * Update packages/pg-pool/package.json Co-Authored-By: Charmander <[email protected]> Co-authored-by: Charmander <[email protected]>
* Remove password from stringified outputs Theres a security concern where if you're not careful and you include your client or pool instance in console.log or stack traces it might include the database password. To widen the pit of success I'm making that field non-enumerable. You can still get at it...it just wont show up "by accident" when you're logging things now. The backwards compatiblity impact of this is very small, but it is still technically somewhat an API change so...8.0. * Implement feedback * Fix more whitespace the autoformatter changed * Simplify code a bit * Remove password from stringified outputs (#2070) * Keep ConnectionParameters’s password property writable `Client` writes to it when `password` is a function. * Avoid creating password property on pool options when it didn’t exist previously. * Allow password option to be non-enumerable to avoid breaking uses like `new Pool(existingPool.options)`. * Make password property definitions consistent in formatting and configurability. Co-authored-by: Charmander <[email protected]>
* Make `native` non-enumerable Making it non-enumerable means less spurious "Cannot find module" errors in your logs when iterating over `pg` objects. `Object.defineProperty` has been available since Node 0.12. See #1894 (comment) * Add test for `native` enumeration Co-authored-by: Gabe Gorelick <[email protected]>
* Use class-extends to wrap Pool * Minimize diff * Test `BoundPool` inheritance Co-authored-by: Charmander <[email protected]> Co-authored-by: Brian C <[email protected]>
…ns (#2076) * Add failing test for creating a `BoundPool` from another instance’s settings * Continue support for creating a pg.Pool from another instance’s options by dropping the requirement for the `password` property to be enumerable.
Not entirely backwards-compatible.
If a client is not queryable, the pool should prevent requeuing instead of strictly enforcing errors to be propagated back to it.
Some weird behavior changed w/ async iteration in node 13.7...I'm not sure if this was an unintentional break or not but it definitely diverges in behavior from node 12 and earlier versions in node 13...so for now going to run tests on 13.6 to unblock the tests from running while I track this down.
charmander
approved these changes
Jan 24, 2020
| @@ -10,7 +10,7 @@ env: | |||
| node_js: | |||
| - lts/dubnium | |||
| - lts/erbium | |||
| - 13 | |||
| - 13.6 | |||
There was a problem hiding this comment.
the test to iterate a stream multiple times w/ async iterator stopped working in node 13.7....I've had occasional breaks before on single semver minor versions of non LTS node...if this doesn't resolve itself w/ 13.8 I'll have to track it down & see if it was something they intentionally changed w/ async iterators on streams.
There was a problem hiding this comment.
btw...found the issue:
| @@ -256,7 +260,7 @@ class Pool extends EventEmitter { | |||
|
|
|||
| client.release = (err) => { | |||
| if (released) { | |||
| throw new Error('Release called on client which has already been released to the pool.') | |||
| throwOnDoubleRelease() | |||
brianc
force-pushed
the
bmc/no-return-broken-client
branch
2 times, most recently
from
b8e7dd2 to
98dd655
Compare
|
having a bit of a mental breakdown trying to rebuild the history in a way that cleanly merges into master. I know it can be done, but for the sake of my own sanity I'm just going to open a new PR that replaces this one, with the same commits, but targets |
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.
Building off of brianc/node-pg-pool#119
This was slated to go into 8.0, but after working on this I'm not sure it's a breaking change...previously if you released a broken or ended client you'd basically put your app into a state where you'd crash or hit undefined behavior in the future. Perhaps we just merge this into the existing 7.x branch so more folks can get the change sooner. what you think @charmander?
Either way I think this closes the final thing mentioned in #2047
The only thing really left for 8.0 is to get some proper tests in place for handling self-signed versus "real" certificates so we can check the code is doing the right thing in travis in all cases. I need to reference the jdbc postgres repo and find how the configure all those different situations to test against them. After that, I can write tests, make sure it works, and release 8.0. Then the fun work of 9.0 begins. 🏃