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

pool: connection ending in fatal error are not removed from the generic pool #452

Closed
syndr0m opened this Issue Sep 1, 2014 · 10 comments

Comments

Projects
None yet
8 participants
@syndr0m

syndr0m commented Sep 1, 2014

when having a fatal error on a mysql connection using knex 0.6.22 & pool & mysql driver, the connection is not removed from the pool.

Further requests using this connection end in mysql driver error "cannot enqueue ... after fatal error" (PROTOCOL_ENQUEUE_AFTER_FATAL_ERROR)

a workaround :

var knex;

var connectionErrorHandler = function (connection, err) {
  if (connection && err && err.fatal) {
    if (connection.removedFromThePool)
      return;
    connection.removedFromThePool = true;
    knex.client.pool.genericPool.destroy(connection);
  }
};

var config =  {
  client: 'mysql',
  (...)
  pool: {
    (...)
    afterCreate: function (connection, callback) {
      connection.on('error', connectionErrorHandler.bind(null, connection));
      connection.on('end', connectionErrorHandler.bind(null, connection));
      callback();
    }
  }
};

knex = knexInit(config);

it might be linked to #206

@tgriesser tgriesser closed this in b73a85f Oct 1, 2014

@elliotf

This comment has been minimized.

Contributor

elliotf commented Nov 24, 2014

@tgriesser as we discussed in IRC, this bug wasn't fixed.. should it be reopened, or should I create a new bug?

Just in case, here's a summary of my findings:
It appears that knex.client.pool.destroy(connection) will only destroy idle connections. The connection that errored is then re-added to the pool inside of Client.prototype.releaseConnection in lib/client.js

One option would be to have client's .releaseConnection check for connection.__knex_disposed and avoid releasing it back into the pool if that is defined. That doesn't seem to be very clean, but I'll see if it works.

Also, I'm not sure where to put the test associated with this. Would a PR include a test/unit/client.js ?

@jonathandelgado

This comment has been minimized.

jonathandelgado commented Nov 24, 2014

+1 on re-open, just experienced this.

elliotf pushed a commit to elliotf/knex that referenced this issue Nov 24, 2014

@tgriesser tgriesser reopened this Nov 24, 2014

elliotf added a commit to elliotf/knex that referenced this issue Nov 24, 2014

Do not add bad connections back to pool
See tgriesser#452.

This is a hacky workaround proof-of-concept conversation starter that
fixes the issue, but should be done in a cross-dialect way, and it
should also include tests.
@elliotf

This comment has been minimized.

Contributor

elliotf commented Nov 24, 2014

Proof of concept fix is in #578.

elliotf added a commit to shutterstock/knex that referenced this issue Dec 2, 2014

Do not add bad connections back to pool
See tgriesser#452.

This is a hacky workaround proof-of-concept conversation starter that
fixes the issue, but should be done in a cross-dialect way, and it
should also include tests.
@wubzz

This comment has been minimized.

Collaborator

wubzz commented May 19, 2016

The recent changes to pool handling in knex from 0.9 and onwards seems to have addressed most issues. We also discovered this kind of behaviour specifically for mysql2.

I'm going to close this and consider it resolved, but feel free to add a new issue if the error persists.

@wubzz wubzz closed this May 19, 2016

@NemoAlex

This comment has been minimized.

NemoAlex commented Nov 10, 2017

I just experienced this error on 0.14.0.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 10, 2017

@NemoAlex do you have some test code to reproduce that?

@shwuhk

This comment has been minimized.

shwuhk commented Nov 15, 2017

I have encountered into this issue too.
We use mysql with pool. This error happen when the connection is idle for a long time(like more than ten minutes) and then we do some query.
I suspect that knex do not handle a timeout connection properly.
Any ideas?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 16, 2017

@shwuhk I would recommend to not use 0.14 until 0.14.1 is released. There are couple of nasty issues which were missed when generic-pool was updated. I'm working on those and hope to get 0.14.1 out this week.

@shwuhk

This comment has been minimized.

shwuhk commented Nov 17, 2017

I would try it again then when 0.14.1 is out. Thanks.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 20, 2017

@shwuhk 0.14.1 it is out

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