Remove callback layer to catch sync connection exceptions

[jafl]

This PR restructures PromisePool's query and execute so they catch synchronous exceptions.

I don't think Connection.query actually throws synchronous exceptions, so we could remove the changes to support PromisePool.query, but I felt that would be going only half way. Why doesn't Connection.query throw exceptions like Connection.execute?

Alternative to #931

[jafl]

I tried merging the latest from master, but the precommit hook does a major reformatting job. This happens even when I pull a clean copy without any pre-existing node_modules directory. How can I get around this?

[th317erd]

Any idea when this PR will be merged? I am having the same issue where the Promise version is not catching this error and I just get an app crash

[sidorares]

@th317erd can you show some repro code for this behaviour?

@jafl sorry I missed this pr. One thing I don't like is that you want to duplicate pool connection release logic in promise wrapper while currently it's delegated to base implementation

[th317erd]

@sidorares I don't think any reproducible code needs to be shown. The situation is simple: A throw is happening inside a nextTick (without a try/catch in internal code). This will always result in an app-crash (unless I am mistaken?).

Here is a backtrace:

connection.js:575
          throw new TypeError(
          ^

TypeError: Bind parameters must not contain undefined. To pass SQL NULL specify JS null
    at options.values.forEach.val (/home/wyatt/Projects/work/eVault-Node/node_modules/mysql2/lib/connection.js:575:17)
    at Array.forEach (<anonymous>)
    at PoolConnection.execute (/home/wyatt/Projects/work/eVault-Node/node_modules/mysql2/lib/connection.js:567:22)
    at getConnection (/home/wyatt/Projects/work/eVault-Node/node_modules/mysql2/lib/pool.js:163:31)
    at process.nextTick (/home/wyatt/Projects/work/eVault-Node/node_modules/mysql2/lib/pool.js:44:37)
    at process.internalTickCallback (internal/process/next_tick.js:70:11)

And, just for kix, this will do it:

var mysql2 = require('mysql2/promise');
var pool = mysql2.createPool(...);

(async function() {
  try {
    await pool.execute('some statement ?', [ undefined ]);
  } catch (e) {
    console.log('It would be great if I could catch the ball!');
  } finally {
    await pool.end();
  }
})();

Ball dropped... hard. :)

As a workaround for now I just added my own validation and throws before I ever call execute... but still, a fix would be nice.

[th317erd]

Further, this is the problem: pool.js

getConnection(cb) {
    if (this._closed) {
      return process.nextTick(() => cb(new Error('Pool is closed.')));
    }
    let connection;
    if (this._freeConnections.length > 0) {
      connection = this._freeConnections.shift();
      this.emit('acquire', connection);
      return process.nextTick(() => cb(null, connection));
    }
....

The return process.nextTick(() => cb(null, connection)); needs to instead be return process.nextTick(() => { try { cb(null, connection); } catch (e) { cb(e, null); } });

[sidorares]

I'm happy to add extra try/catch around cb invocations, especially that now it's much cheaper in V8 compared to 6 years ago

[jafl]

The return process.nextTick(() => cb(null, connection)); needs to instead be return process.nextTick(() => { try { cb(null, connection); } catch (e) { cb(e, null); } });

How does this help with catching exceptions related to undefined values?

[jafl]

@jafl sorry I missed this pr. One thing I don't like is that you want to duplicate pool connection release logic in promise wrapper while currently it's delegated to base implementation

I wasn't happy with it, either, but I couldn't see any other way to make the exceptions catchable. Duplicating the logic removes the extra layer, so exceptions propagate correctly.

[sidorares]

@jafl yeah, I see your point

can you try to check where error stack points with your changes when you run this

(async function() {
  try {
    await pool.execute('select 1+?', [ undefined ]);
  } catch (e) {
    console.log('It would be great if I could catch the ball!');
  } finally {
    await pool.end();
  }
})();

also would be good to add unit test

[jafl]

Sorry for taking so long. Life got in the way...

I added your code as a test in test-promise-wrappers.js, and it prints:

  It would be great if I could catch the ball!
  error: TypeError: Bind parameters must not contain undefined. To pass SQL NULL specify JS null
      at /Users/jolindal/tools/node-mysql2/lib/connection.js:602:17
      at Array.forEach (<anonymous>)
      at PoolConnection.execute (/Users/jolindal/tools/node-mysql2/lib/connection.js:594:22)
      at /Users/jolindal/tools/node-mysql2/promise.js:110:11
      at new Promise (<anonymous>)
      at PromisePoolConnection.execute (/Users/jolindal/tools/node-mysql2/promise.js:107:12)
      at /Users/jolindal/tools/node-mysql2/promise.js:350:19
      at processTicksAndRejections (internal/process/task_queues.js:93:5)
      at async testCatchSyncUndefinedError (/Users/jolindal/tools/nodemysql2/test/integration/promise-wrappers/test-promise-wrappers.js:452:5)```

It looks like it works.

[jafl]

Have you had a chance to review the diffs?

[sidorares]

Thanks @jafl! could you fix link errors here? Also, is there any unit test that touches the issue that this PR fixes?

[jafl]

Both builds are complaining about indentation - but that is caused by the pre-commit hook.

[jafl]

I added the unit test in commit 70b75a7

[jafl]

How do I force the pre-commit hook to indent the way lint wants?

[jafl]

Build is fixed.

[jafl]

What else needs to be done before this PR can be merged?

[sidorares]

hey @jafl , the PR probably looks good, I just need to find time to review it as it is quite big

[SebastianZaha]

I wanted to use @jafl 's fix branch but it seems if call getConnection() on a pool, the query method on that connection expects a pool parameter as first argument instead of the query string. This, for example crashes: let conn = await pool.getConnection(); conn.query('BEGIN');

(later edit: testing by requiring mysql2/promise)

[jafl]

Thank you for catching this! I have adjusted it.

[leibale]

Is this PR going to be merged into master?

[sidorares]

@leibale yes

[Mikapsien]

My naive sugestion is to return cb(e) in the catch clause of execute in pool.js. It will make our code simpler unlike now manually checking for all values.

[jafl]

My naive sugestion is to return cb(e) in the catch clause of execute in pool.js. It will make our code simpler unlike now manually checking for all values.

Can you provide more details? What values are you checking for? Can you provide example code?

[jafl]

Is this PR still useful, or is it obsolete?

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 87.50000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 90.31%. Comparing base (e87ffb7) to head (273e632).

Files	Patch %	Lines
promise.js	87.50%	4 Missing :warning:
lib/pool.js	85.71%	2 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #938      +/-   ##
==========================================
- Coverage   90.33%   90.31%   -0.02%     
==========================================
  Files          71       71              
  Lines       15731    15749      +18     
  Branches     1344     1350       +6     
==========================================
+ Hits        14210    14224      +14     
- Misses       1521     1525       +4     

Flag	Coverage Δ
compression-0	90.12% <87.50%> (-0.22%)	:arrow_down:
compression-1	90.31% <87.50%> (-0.02%)	:arrow_down:
tls-0	89.84% <87.50%> (-0.02%)	:arrow_down:
tls-1	90.12% <87.50%> (-0.02%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jafl]

@sidorares I cleaned up the PR for you to review again.