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

Use tarn as pool #2450

Merged
merged 5 commits into from Feb 7, 2018

Conversation

Projects
None yet
7 participants
@koskimas
Collaborator

koskimas commented Feb 2, 2018

Hi,

This experimental PR replaces the current pool generic-pool with Tarn. There has been a bunch of discussion about this change in the following issues: #2339 #1702

At this point the pull request is here mainly so that some brave soul, who is having problems with the current pool, could test this out to see if it solves their problems.

Also fixes #2442

let promise = null
if (this.pool) {
promise = this.pool.destroy()

This comment has been minimized.

@koskimas

koskimas Feb 2, 2018

Collaborator

destroy drains and clears the pool just like drain + clear with the old pool.

@koskimas

This comment has been minimized.

Collaborator

koskimas commented Feb 2, 2018

Oh, knex still tests against node 4 and 5. Tarn currently requires 6. That should be easy to fix. There's something else wrong with the tests too. Funny that they all passed on my computer 😄 I'll look into it.

@igor-savin-ht

This comment has been minimized.

Contributor

igor-savin-ht commented Feb 2, 2018

@koskimas Node 5 is EOL already, Node 4 wil be in 2 months. Do we still want to support them?

@koskimas

This comment has been minimized.

Collaborator

koskimas commented Feb 2, 2018

@igor-savin-ht It's not up to me. There are only a couple of things in Tarn that require 6. I can easily change those.

@igor-savin-ht

This comment has been minimized.

Contributor

igor-savin-ht commented Feb 2, 2018

@elhigu Any plans to drop Node 4/5 support in the nearby future? Given that Node.js migration path is really smooth compared to other stacks, I wonder if there are any reasons to support legacy versions for a long time.

@koskimas

This comment has been minimized.

Collaborator

koskimas commented Feb 2, 2018

There's one test that fails and only on oracle

oracledb - acquireConnectionTimeout works

I have no idea what that's about. @elhigu Any ideas?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 2, 2018

I think we can drop node 4 already () and later on drop support always when node version reach EOL. @koskimas no idea why that could be failing... it haven't failed before, need to check if test is done wrong or if that timeout is really not working with oracle...

@igor-savin-ht

This comment has been minimized.

Contributor

igor-savin-ht commented Feb 2, 2018

@elhigu Created a PR for that. Do you want anything in knex/documentation changed as well?

@koskimas

This comment has been minimized.

Collaborator

koskimas commented Feb 2, 2018

@elhigu I changed that test so I probably broke it, but I did nothing oracle specific in this PR. I'll try to debug that later.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 3, 2018

@igor-savin-ht yes please. I also discussed with @koskimas about the changing this PR to work with Node 4 and 5 after all so that there will be at least one version in knex 0.14.x that works and that will be last version supporting node 4.

@koskimas

This comment has been minimized.

Collaborator

koskimas commented Feb 4, 2018

@elhigu Tests now pass

@koskimas koskimas changed the title from WIP: Use tarn as pool to Use tarn as pool Feb 4, 2018

@elhigu elhigu referenced this pull request Feb 5, 2018

Closed

Memory leak in 0.14.x #2383

@elhigu

elhigu approved these changes Feb 5, 2018

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 5, 2018

Code looked nice 👍 I will merge / release 0.14.3 after running some more time stress testing.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 5, 2018

knex-0.14.2 master:
knex-0 14 2 master

15mins 100MB -> ~250MB

tarn-branch:
tarn-branch

45mins stable ~105MB

@koskimas

This comment has been minimized.

Collaborator

koskimas commented Feb 5, 2018

@elhigu I added warnings about the old generic-pool config options

koskimas added some commits Feb 2, 2018

destroy connection if a query times out (and cancellation is not enab…
…led)

Before this change, when a query timeout happened without { cancel: true }
the connection was immediately released back to the pool. If the query
timed out because of a time consuming query, the released connection would
still execute the query to its end and the connection would be useless until
that time. This commit marks the connection as disposed so that the pool never
again gives that connection to anyone, and destorys it in the near future.

@elhigu elhigu merged commit 8771bd4 into tgriesser:master Feb 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@koskimas

This comment has been minimized.

Collaborator

koskimas commented Feb 10, 2018

Now that this is merged, who should I give write access to tarn? I'll of course help maintain it too, but I also want to give full access for knex maintainers. @elhigu already has access.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 10, 2018

At least for @wubzz and @tgriesser I think. For the rest of the people access may be given on demand when they happen to need them.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Feb 13, 2018

I believe we also need to update the Upgrading section of the docs:

Upgrading 0.13 -> 0.14
generic-pool was upgraded to v3. If you have specified idleTimeoutMillis or softIdleTimeoutMillis in the pool config then you will need to add evictionRunIntervalMillis: 1000 when upgrading to 0.14.

See original issue #2322 for details.

Since technically 0.13 -> 0.14 now includes not only generic-pool v3 but also tarn.js 1.1.2. For better or for worse tarn.js uses the old idleTimeoutMillis instead of evictionRunIntervalMillis (don't get me wrong, I definitely prefer the old property), so the upgrading guide needs to say the reverse of what it's saying right now (evictionRunIntervalMillis back to idleTimeoutMillis).

Scratch that. Should actually be able to just remove the text all together since idleTimeoutMillis was still a requirement, while evictionRunIntervalMillis now becomes moot.

Just want to double-check with @koskimas @elhigu that I'm right about this before I make an issue in docs repo.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 13, 2018

Yup I think we should remove that upgrading section and add warning about using versions 0.14.0 - 0.14.2

@pablopen

This comment has been minimized.

Contributor

pablopen commented Feb 16, 2018

I hope you dont drop support for node 4. I know it only has 2 months left for LTS, but it's really neccessary?
And yes, this will affect me :( I'm using node 4.6 with oracledb right now, and node migration doesn't depend on me,

@igor-savin-ht

This comment has been minimized.

Contributor

igor-savin-ht commented Feb 16, 2018

@pablopen Given how smooth and painless is node upgrade process is, what is the rationale of whoever is responsible for making the decision not to do it? We can't support obsolete versions forever, so sooner or later update needs to happen anyway.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 16, 2018

@pablopen it wont be dropped before 0.15 versions. 0.14.4 will be most stable knex released so far you can still use it after node 4 is deprecated. I’m pretty sure oracledb will drop support for node 4 too around the same time (they already dropped 5 and 7). So if you are not able to upgrade node you need to relay in older package versions.

@bertho-zero

This comment has been minimized.

bertho-zero commented Apr 23, 2018

The doc is not updated here: http://knexjs.org/#Installation-pooling

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 23, 2018

@bertho-zero good catch. Thanks

@irsl

This comment has been minimized.

Contributor

irsl commented Apr 29, 2018

Only partially related: I'm wondering if the Sqlite adapter could now be improved so that read-only queries would not be blocked anymore while a transaction is in progress? In this aspect, knex.js performs worse than using node-sqlite3 directly.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 1, 2018

@irsl AFAIK If you add more connections to the pool than just one, there is no blocking in knex side. Please open an issue if you think that knex is blocking multiple connections. IIRC sqlite allows more connections that just one nowadays (except for inmemory DB).

@irsl

This comment has been minimized.

Contributor

irsl commented May 3, 2018

@elhigu I just verified, 0.14.6 still holds back the read queries while a transaction is in progress. Note: using the Sqlite library directly this just works fine.

I managed to hook Knex.js 0.12.6 back then to address this, but it was so ugly at the end I decided not to contribute it. My patches obviosly broke with the recent changes in Knex.
Anyways, I just created a small mini-project with my unit tests to see what my expectations are:

irsl/knex-sqlite-transactions@ec22313

@irsl

This comment has been minimized.

Contributor

irsl commented May 3, 2018

Sorry, I was inattentive regarding your remark on the pools. By increasing the defaults and tweaking a bit in one of my tests everything works as expected. Great!

@irsl

This comment has been minimized.

Contributor

irsl commented Jun 17, 2018

@elhigu I just upgraded to the latest Knex, and started encountering SQLITE_BUSY errors without my custom transaction hooks (when the pool number is higher than 1). I also learned the node-sqlite3 library lacks busy timeout support (mapbox/node-sqlite3#9) and as I see it won't change as it would block the complete applciation due to the way how Node works.

My question is:

To workaround this, do you think a transaction serialization feature could be added in Knex? My original patch for 0.12.6 looked something like this:

        if(!settings.officialSinglePoolMode) {
            const bluePromise = knex.Promise;
            var transactionWaitList = [];
            var originalTransaction = knex.transaction;
            knex.transaction = function(callback){
                if(debug) console.log("!!! transaction was called");

                return new bluePromise(function(resolve,reject){

                    transactionWaitList.push({callback:callback, resolve: resolve, reject: reject});

                    // initial kickoff:
                    if(transactionWaitList.length == 1)
                       fireNextOne();

                });

                function fireNextOne(){
                      // are there any more transactions?...
                      if(transactionWaitList.length <= 0) return;

                      var stuff = transactionWaitList[0];
                      var oex;
                      return originalTransaction(stuff.callback)
                      .catch(ex=>{
                          // console.log("there was an exception!", ex.message)
                          oex = ex;
                      })
                      .then((returnValue)=>{
                         if(debug) console.log("!!! original transaction has finished");
                         transactionWaitList.shift();
                         fireNextOne();

                         if(oex) return stuff.reject(oex)
                         return stuff.resolve(returnValue);
                      })
                }

            }

        }
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jun 27, 2018

Transactions should be automatically serialized if you have set pool size to be 1, because when transaction is started it tries to acquire connection from pool and if there are no connections available, knex will wait until the previous transaction returns the connection back to the pool.

So I'm failing to see the problem here that you are trying to workaround...

@irsl

This comment has been minimized.

Contributor

irsl commented Jun 27, 2018

Pool size 1 limits the number of concurrent read operations (eg. select) as well. Sqlite is better than that.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jun 27, 2018

@irsl So why are you getting SQLITE_BUSY errors? You think that only serializing transactions that there are no multiple of them running simultaneously would prevent that once and for all?

I'm still failing to see the problem here that you are trying to workaround... Also this is wrong place to discuss about this. If you could create new issue about functionality you are experiencing with some way to reproduce your problems, that could be useful for thinking of solution...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment