Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert to generic pool #1665

Merged
merged 2 commits into from Sep 13, 2016
Merged

Revert to generic pool #1665

merged 2 commits into from Sep 13, 2016

Conversation

tgriesser
Copy link
Member

For 1.0, I'm planning on making the pool optional or easily configurable by allowing folks to easily inject the strategy for managing connections in the configuration.

In the meantime there's quite a few issues that keep cropping up with Pool2, and as nice as the features like capabilities and backoffs are in that library, I think for now I'm going to keep things a bit simpler with node-pool.

In looking at node-pool it seems to have developed to be a bit nicer since last time we had it in here as a pooling lib a few years back. So for 0.12 I'm proposing we go with generic-pool and then in 1.0 we'll use the pools that come with the individual adapter libraries, with a documented way to replace with something like generic-pool or pool2.

@tgriesser tgriesser merged commit 7069ce5 into master Sep 13, 2016
@tgriesser tgriesser deleted the generic-pool branch September 13, 2016 22:12
tgriesser added a commit that referenced this pull request Sep 14, 2016
* master:
  release 0.12.0
  Remove docs, in favor of https://github.com/knex/documentation (#1666)
  Revert to generic pool (#1665)
  Fix #1619
  Fix use of const in test suite for node 0.12
  Moving bin/cli outside of src to allow install from master
  Deprecate Knex.Promise
  Simplifying internal client structure
  add With Clause (#1599)
  Simplify transaction classes
  Simplify formatter use
  Deprecate VERSION, update changelog
  Fix PG string escaping behavior (#1661)
@ErisDS
Copy link
Contributor

ErisDS commented Sep 16, 2016

In the meantime there's quite a few issues that keep cropping up with Pool2

Hey @tgriesser, could you possibly share some details on what the known issues with Pool2 are? Trying to assess whether upgrading to knex 0.12 is critical or not.

@tgriesser
Copy link
Member Author

Basically this issue TryGhost/Ghost#7346 - if something messes up with the db connection, knex was intercepting the error event emitted by pool2 and just logging it instead of recovering from it properly (or emitting it so it would take down/restart the app). So prior to 0.12 the server can easily get into a bad state.

kirrg001 added a commit to kirrg001/Ghost that referenced this pull request Sep 26, 2016
refs TryGhost#7189
- we had a memory leak after upgrading to knex 0.11.x
- knex has published a new version 0.12.x
- the memory leak does not longer exists
- knex has reverted their pool logic, see knex/knex#1665
kirrg001 added a commit to kirrg001/Ghost that referenced this pull request Sep 29, 2016
refs TryGhost#7189
- we had a memory leak after upgrading to knex 0.11.x
- knex has published a new version 0.12.x
- the memory leak does not longer exists
- knex has reverted their pool logic, see knex/knex#1665
ErisDS pushed a commit to TryGhost/Ghost that referenced this pull request Sep 29, 2016
refs #7189
- we had a memory leak after upgrading to knex 0.11.x
- knex has published a new version 0.12.x
- the memory leak does not longer exists
- knex has reverted their pool logic, see knex/knex#1665
yo1dog pushed a commit to yo1dog/Ghost that referenced this pull request Oct 14, 2016
refs TryGhost#7189
- we had a memory leak after upgrading to knex 0.11.x
- knex has published a new version 0.12.x
- the memory leak does not longer exists
- knex has reverted their pool logic, see knex/knex#1665
mixonic pushed a commit to mixonic/Ghost that referenced this pull request Oct 28, 2016
refs TryGhost#7189
- we had a memory leak after upgrading to knex 0.11.x
- knex has published a new version 0.12.x
- the memory leak does not longer exists
- knex has reverted their pool logic, see knex/knex#1665
geekhuyang pushed a commit to geekhuyang/Ghost that referenced this pull request Nov 20, 2016
refs TryGhost#7189
- we had a memory leak after upgrading to knex 0.11.x
- knex has published a new version 0.12.x
- the memory leak does not longer exists
- knex has reverted their pool logic, see knex/knex#1665
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants