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

Unhandled Exception in `connection.end()` for MySQL Dialect #1691

Closed
brycekbargar opened this Issue Sep 24, 2016 · 2 comments

Comments

Projects
None yet
2 participants
@brycekbargar

brycekbargar commented Sep 24, 2016

There's an open issue on the mysql repo for ClearDB not responding the a Quit packet.

This affects knex when destroying an idle connection using Client_MySQL.destroyRawConnection().

  destroyRawConnection(connection) {
    // This removes the error handler attached by knex in `aquireRawConnection()`
    // It also removes any error handlers attached by the end user
    connection.removeAllListeners()
    // In the mysql library this takes an error callback, or alternatively emits the 'error' event
    connection.end()
  }

The unhandled exception thrown by mysql on connection.end() was crashing my application and there is really no way to handle it as knex is currently setup.

I would be willing to open a PR and fix the bug, but I'm not sure which strategy is most appropriate.

The easiest would be to remove the call to removeAllListeners(), according to the NodeJS docs

"Note that it is bad practice to remove listeners added elsewhere in the code, particularly when the EventEmitter instance was created by some other component or module (e.g. sockets or file streams)."

That said, I'm assuming there is a reason knex is calling this method? Maybe something to do with performance?

An alternative would be to pass in an error callback to `.end()', for example:

  destroyRawConnection(connection) {
    connection.removeAllListeners()
    connection.end(err => {
      connection.__knex__disposed = err
    })
  }

Let me know which approach would be best and I'll make the change and open a PR.

As a workaround, for anyone else experiencing issues with knex, heroku, mysql, and cleardb I've done the following and it works for now but is fairly ugly...

config.pool.afterCreate = (conn, cb) => {
  const removeAllListeners = conn.removeAllListeners;
  conn.removeAllListeners = () => {
    removeAllListeners.call(conn);
    conn.on('error', err => conn.__knex__disposed = err);
  };
  cb(null);
};
@tgriesser

This comment has been minimized.

Owner

tgriesser commented Sep 27, 2016

The unhandled exception thrown by mysql on connection.end() was crashing my application and there is really no way to handle it as knex is currently setup.

Yikes, sorry about that.

That said, I'm assuming there is a reason knex is calling this method? Maybe something to do with performance?

Basically it's there just to clean up any listeners hanging around that aren't needed anymore since the connection shouldn't be accessible for any purpose once it's "destroyed" anyway.

I think this would be the preferred solution:

connection.end(err => {
  err.__knex__disposed = err
})

or maybe even:

destroyRawConnection(connection) {
  connection.end(() => {
    connection.removeAllListeners()
  })
}

so that any error handler still exists until after the connection is definitely ended.

@tgriesser tgriesser added the bug label Sep 27, 2016

tgriesser added a commit that referenced this issue Sep 27, 2016

tgriesser added a commit that referenced this issue Sep 27, 2016

Merge branch '0.12.x'
* 0.12.x:
  release 0.12.2
  Update changelog for 0.12.2 features
  Don't force master as release branch
  Remove unused pool2 dependency
  Fix #1701
  Fix #1675
  Fix for #1691
@tgriesser

This comment has been minimized.

Owner

tgriesser commented Sep 27, 2016

Just pushed a fix in 0.12.2, thanks for the report!

@tgriesser tgriesser closed this Sep 27, 2016

tgriesser added a commit that referenced this issue Oct 9, 2016

Merge branch 'master' into refactor
* master:
  release 0.12.3
  Update changelog
  Fix #1710
  Fix #1694
  release 0.12.2
  Update changelog for 0.12.2 features
  Don't force master as release branch
  Remove unused pool2 dependency
  Fix #1701
  Fix #1675
  Fix for #1691

RubenSlabbert added a commit to RubenSlabbert/knex that referenced this issue Nov 9, 2016

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