Node-mysql2: uncatchable `uncaughtException` in `pool.execute` if `undefined` in query values

[dr-js]

undefined in query values will cause TypeError: Bind parameters must not contain undefined. To pass SQL NULL specify JS null

with pool.execute, is there a place to catch the error?

the simplified test script:

// test-pool-execute-error.js

const Mysql = require('mysql2')

const CONFIG = {
  host: '127.0.0.1',
  port: 3306,
  user: 'root',
  database: 'mysql'
}

const testNormal = () => {
  console.log('[testNormal] start')
  const pool = Mysql.createPool(CONFIG)
  pool.execute(
    `SET @value = ?`,
    [ 0 ], // good value
    (error, resultList, fieldList) => console.log('[testNormal] from callback:', { error, resultList, fieldList })
  )
  setTimeout(() => pool.end(() => console.log('[testNormal] pool ended')), 1000)
}

const testError = () => {
  try {
    console.log('[testError] start')
    const pool = Mysql.createPool(CONFIG)
    pool.on('error', (error) => console.log('[testError] error from event', error))
    pool.execute(
      `SET @value = ?`,
      [ undefined ], // bad value
      (error, resultList, fieldList) => console.log('[testError] from callback:', { error, resultList, fieldList })
    )
    setTimeout(() => pool.end(() => console.log('[testError] pool ended')), 1000)
  } catch (error) { console.log('[testError] error from catch', error) }
}

process.on('uncaughtException', (error) => console.error('!!! error from uncaughtException', error))

testNormal()

setTimeout(() => { // wait first test pass
  testError()
}, 2000)

the output with [email protected]: (testNormal pass, but testError throws and block the process exiting)

$ node test-pool-execute-error.js
[testNormal] start
[testNormal] from callback: { error: null,
  resultList:
   ResultSetHeader {
     fieldCount: 0,
     affectedRows: 0,
     insertId: 0,
     info: '',
     serverStatus: 2,
     warningStatus: 0 },
  fieldList: undefined }
[testNormal] pool ended
[testError] start
!!! error from uncaughtException TypeError: Bind parameters must not contain undefined. To pass SQL NULL specify JS null
    at ...\node_modules\mysql2\lib\connection.js:609:17
    at Array.forEach (<anonymous>)
    at PoolConnection.execute (...\node_modules\mysql2\lib\connection.js:607:22)
    at ...\node_modules\mysql2\lib\pool.js:171:31
    at Pool.<anonymous> (...\node_modules\mysql2\lib\pool.js:68:18)
    at PoolConnection.<anonymous> (...\node_modules\mysql2\lib\connection.js:753:13)
    at Object.onceWrapper (events.js:273:13)
    at PoolConnection.emit (events.js:182:13)
    at ClientHandshake.<anonymous> (...\node_modules\mysql2\lib\connection.js:108:20)
    at ClientHandshake.emit (events.js:182:13)

[sidorares]

At the moment it's not possible to catch with pool.execute() because internally it does async pool.getConnection() + sync throw from connection.execute().

I think you can catch this error if you split call in two, but I suggest fixing root cause first (filter / map parameters)

 pool.getConnection((err, conn) => {
    try {
      conn.execute(
        `SET @value = ?`,
        [ undefined ], // bad value
        (error, resultList, fieldList) => console.log('[testError] from callback:', { error, resultList, fieldList })
      )
    } catch(e) {
      console.log('this is expected: ', e);
    }
})

[ThatBean]

one possible fix may be add one additonal try/catch in lib/pool.js around L152:

  execute(sql, values, cb) {
    // TODO construct execute command first here and pass it to connection.execute
    // so that polymorphic arguments logic is there in one place
    if (typeof values == 'function') {
      cb = values;
      values = [];
    }
    this.getConnection(function(err, conn) {
      if (err) {
        return cb(err);
      }
+      try {
      const executeCmd = conn.execute(sql, values, cb);
      executeCmd.once('end', function() {
        conn.release();
      });
+      } catch(err) {
+        return cb(err);
+      }
    });
  }

this will change the test output to:

$ node test-pool-execute-error.js
[testNormal] start
[testNormal] from callback: { error: null,
  resultList:
   ResultSetHeader {
     fieldCount: 0,
     affectedRows: 0,
     insertId: 0,
     info: '',
     serverStatus: 2,
     warningStatus: 0 },
  fieldList: undefined }
[testNormal] pool ended
[testError] start
[testError] from callback: { error:
   TypeError: Bind parameters must not contain undefined. To pass SQL NULL specify JS null
       at ...\node_modules\mysql2\lib\connection.js:609:17
       at Array.forEach (<anonymous>)
       at PoolConnection.execute (...\node_modules\mysql2\lib\connection.js:607:22)
       at ...\node_modules\mysql2\lib\pool.js:172:33
       at Pool.<anonymous> (...\node_modules\mysql2\lib\pool.js:68:18)
       at PoolConnection.<anonymous> (...\node_modules\mysql2\lib\connection.js:753:13)
       at Object.onceWrapper (events.js:273:13)
       at PoolConnection.emit (events.js:182:13)
       at ClientHandshake.<anonymous> (...\node_modules\mysql2\lib\connection.js:108:20)
       at ClientHandshake.emit (events.js:182:13),
  resultList: undefined,
  fieldList: undefined }
[testError] pool ended

but I don't know if extra try/catch will cause performance hit

[sidorares]

but I don't know if extra try/catch will cause performance hit

it used to be a hit previously, maybe less so now with modern v8. I think the intention was to treat this kind of errors less like runtime and more like "design time" error when you forget some parameter or thinks like this, thus it's better to error hard and early

How you encountered error in your case? Did you try to pass undefined deliberately?

[dr-js]

It's a node server/worker for read&process data in mysql.

This was discovered during early testing with some loose data code.
I do agree undefined in data should be handled before the query,
and the code will be added later..

In my case the server will crash&restart on uncaughtException/unhandledRejection,
so I try to add catch for most error.

[dr-js]

Mostly this issue is for noting this uncaughtException behavior in pool.execute,
and some ways to prevent it.

I know if we want to patch this, a lot more test may be needed, and that takes time.
This does not block our usage of mysql2, and feel free to close this issue.

[knoxcard]

@dr-js, go ahead and close the ticket, you are the one who is able to do that.

[damianobarbati]

Why was this closed? An unhandled rejection cause process exit/crash.
This is pretty serious; there's no way to catch this error, no matter where you put the try/catch .

@dr-js how did you solve?

[dr-js]

@damianobarbati this issue is closed for a while and things may be changed

As far I remember it's avoidable if you knew this and make sure the input value do not have undefined
Like add a wrapper function to do a value check

[theirongiant]

This isn't so straightforward when you're using namedParameters and passing in an object, you'd need to maintain a list of expected keys to check for each query. As far as I can tell ThatBean's fix works, if the difference is between killing the server vs a potential performance hit would it be possible to add an alternative safe version of execute so we can choose which to use?

[hktr92]

it just happened to me, too. in a project that is supposed to go live sooner or later.

more time is wasted to overcome this issue by this method (implementing a strict type conversion + restart policy for a fallback in business logic) rather than having a try / catch in the library itself... which can be further caught and handled properly.

if the intention is to report an issue, then throw an error, don't kill the process. it's bad design imho and raises uncertainty for production.

also, the error reported is pretty ambiguous and doesn't really tell what query caused this issue, which makes harder to debug + repair quickly (for reference, Doctrine (PHP library) reports which query failed in dev mode).