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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Database connection is not correctly destroyed after knex 0.12.2 #1973

Closed
kirrg001 opened this Issue Mar 15, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@kirrg001
Copy link
Collaborator

kirrg001 commented Mar 15, 2017

Hey 馃憢

I have recently updated knex from 0.12.2 to 0.12.7 and noticed that the database connection is not closed when calling connection.destroy. The result is, that a process doesn't exit.

Here is a snippet:

....
this.connection = knex(...);

return this.connection.transaction(function(txn) {
   return doSomething(txn)
       .finally(function() {
           return self.connection.destroy()
                .then(function () {
                    debug('Destroyed connection');
                });
       });
})

NOTE: debug('Destroyed connection') get's called.

I have tested all knex versions and it starts in 0.12.3.

The code works for me with 0.12.2, but does not with 0.12.3.
Is there anything i can change in my code to get it working with > 0.12.2?

Otherwise, I went through the changes, maybe related to the pool update #1702?

Thank you so much!
Kate

@wubzz

This comment has been minimized.

Copy link
Collaborator

wubzz commented Mar 15, 2017

Based on the changelog I would guess #1731 causes this in some way. Some changes to acquireConnection was done so that connections are properly returned to the pool.

Edit: Testing with postgres, I'm unable to reproduce the issue as described, while switching between versions 0.12.2 -> 0.12.7. Viewing the PG server status, the connections appear to close properly and returns to the pool in both versions. The process does not exit in either version.

const db = require('knex')({...});

db
.transaction((tr) => tr('user').select())
.then(() => db.destroy())
.then(() => console.log('Destroyed'));
//No open connections in server status
@kirrg001

This comment has been minimized.

Copy link
Collaborator Author

kirrg001 commented Mar 15, 2017

I have tested with MySQL only. Tested multiple mysql versions.

@kirrg001

This comment has been minimized.

Copy link
Collaborator Author

kirrg001 commented Mar 15, 2017

I debugged.

This timeout https://github.com/tgriesser/knex/blob/master/src/client.js#L234 is blocking the process to exit.

@kirrg001

This comment has been minimized.

Copy link
Collaborator Author

kirrg001 commented Mar 15, 2017

https://github.com/tgriesser/knex/blob/master/src/client.js#L243

If an error occurs, shouldn't the timeout be cleared?
This solves my issue. I can raise a PR.

@wubzz

This comment has been minimized.

Copy link
Collaborator

wubzz commented Mar 15, 2017

@kirrg001 Ah yes, makes sense the timeout would cause the process not to exit, but it also makes sense why @tgriesser added it. In order to properly release connections back to the pool, the two handlers must be seperate for TimeoutError and pool.acquire, and so a timeout check is inevitable.

I agree clearTimeout should be called when an error occurs as well.

Does your app's process "eventually" exit, or does it idle beyond acquireConnectionTimeout with its default value of 60000 ms?

Unfortunately I don't have an answer to this problem right now. Will try look into this some more.

@kirrg001

This comment has been minimized.

Copy link
Collaborator Author

kirrg001 commented Mar 15, 2017

Does your app's process "eventually" exit, or does it idle beyond acquireConnectionTimeout with its default value of 60000 ms?

It exits after the timeout.

My use case is a bit complex and 3 database connections are created, one of these connections can throw an error at the moment if the database does not exist, knex tries to acquire a connection to this target database and fails. That's why clearing the timeout solves this issue for me. Thanks for your fast support here.

@wubzz

This comment has been minimized.

Copy link
Collaborator

wubzz commented Mar 15, 2017

@kirrg001 I see, I thought your app remained idle eventhough all attempts of acquiring connections were successful, but I understand now that's not the case.

I'll merge this in right now, definitely a bug. Thanks for the report!

wubzz added a commit to wubzz/knex that referenced this issue Mar 15, 2017

@wubzz wubzz closed this in #1974 Mar 15, 2017

wubzz added a commit that referenced this issue Mar 15, 2017

@wubzz

This comment has been minimized.

Copy link
Collaborator

wubzz commented Mar 15, 2017

@kirrg001 FYI: Pushed 0.12.8

@kirrg001

This comment has been minimized.

Copy link
Collaborator Author

kirrg001 commented Mar 15, 2017

Thanks so much 馃惄

kirrg001 added a commit to TryGhost/knex-migrator that referenced this issue Mar 15, 2017

馃洜 make knex less strict again
closes #59

- minimum knex version is 0.12.8
- see tgriesser/knex#1973
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.