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

Use tarn as pool #2450

Merged
merged 5 commits into from Feb 7, 2018
Merged

Use tarn as pool #2450

merged 5 commits into from Feb 7, 2018

Conversation

@koskimas
Copy link
Collaborator

@koskimas 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
Author Collaborator

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

@koskimas koskimas force-pushed the koskimas:use-tarn-as-pool branch from c23a0fa to 0e9fd45 Feb 2, 2018
@koskimas
Copy link
Collaborator Author

@koskimas 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.

@koskimas koskimas force-pushed the koskimas:use-tarn-as-pool branch from 0e9fd45 to 5df7702 Feb 2, 2018
@igor-savin-ht
Copy link
Contributor

@igor-savin-ht 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
Copy link
Collaborator Author

@koskimas 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
Copy link
Contributor

@igor-savin-ht 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 koskimas force-pushed the koskimas:use-tarn-as-pool branch 2 times, most recently from 0e993d1 to 263ad11 Feb 2, 2018
@koskimas
Copy link
Collaborator Author

@koskimas 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
Copy link
Member

@elhigu 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
Copy link
Contributor

@igor-savin-ht igor-savin-ht commented Feb 2, 2018

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

@koskimas
Copy link
Collaborator Author

@koskimas 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
Copy link
Member

@elhigu 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 koskimas force-pushed the koskimas:use-tarn-as-pool branch 4 times, most recently from c821a80 to 7c6d853 Feb 4, 2018
@koskimas
Copy link
Collaborator Author

@koskimas koskimas commented Feb 4, 2018

@elhigu Tests now pass

@koskimas koskimas changed the title WIP: Use tarn as pool Use tarn as pool Feb 4, 2018
@elhigu elhigu mentioned this pull request Feb 5, 2018
@elhigu
elhigu approved these changes Feb 5, 2018
@elhigu
Copy link
Member

@elhigu elhigu commented Feb 5, 2018

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

@elhigu
Copy link
Member

@elhigu 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 koskimas force-pushed the koskimas:use-tarn-as-pool branch 2 times, most recently from 84f39e6 to 0887b74 Feb 5, 2018
@koskimas
Copy link
Collaborator Author

@koskimas koskimas commented Feb 5, 2018

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

@koskimas koskimas force-pushed the koskimas:use-tarn-as-pool branch from 0887b74 to d548758 Feb 5, 2018
koskimas added 4 commits Feb 2, 2018
…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.
@koskimas koskimas force-pushed the koskimas:use-tarn-as-pool branch from d548758 to 2547f47 Feb 6, 2018
@elhigu elhigu merged commit 8771bd4 into knex:master Feb 7, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@koskimas
Copy link
Collaborator Author

@koskimas 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
Copy link
Member

@elhigu 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
Copy link
Member

@wubzz 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
Copy link
Member

@elhigu 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
Copy link
Contributor

@pablopen 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
Copy link
Contributor

@igor-savin-ht 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
Copy link
Member

@elhigu 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
Copy link
Contributor

@bertho-zero bertho-zero commented Apr 23, 2018

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

@elhigu
Copy link
Member

@elhigu elhigu commented Apr 23, 2018

@bertho-zero good catch. Thanks

@irsl
Copy link
Contributor

@irsl 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
Copy link
Member

@elhigu 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
Copy link
Contributor

@irsl 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
Copy link
Contributor

@irsl 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
Copy link
Contributor

@irsl 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
Copy link
Member

@elhigu 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
Copy link
Contributor

@irsl 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
Copy link
Member

@elhigu 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants