search

sidorares / node-mysql2

:zap: fast mysqljs/mysql compatible mysql driver for node.js
https://sidorares.github.io/node-mysql2/
MIT License
4.07k stars 618 forks source link

Remove callback layer to catch sync connection exceptions #938

Open jafl opened 5 years ago

jafl commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 4 years ago

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 commented 4 years ago

Have you had a chance to review the diffs?

sidorares commented 4 years ago

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

jafl commented 4 years ago

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

jafl commented 4 years ago

I added the unit test in commit 70b75a7

jafl commented 4 years ago

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

jafl commented 4 years ago

Build is fixed.

jafl commented 4 years ago

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

sidorares commented 4 years ago

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

SebastianZaha commented 4 years ago

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 commented 4 years ago

Thank you for catching this! I have adjusted it.

leibale commented 4 years ago

Is this PR going to be merged into master?

sidorares commented 4 years ago

@leibale yes

Mikapsien commented 3 years ago

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 commented 3 years ago

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 commented 6 months ago

Is this PR still useful, or is it obsolete?

codecov[bot] commented 6 months ago

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 ```diff @@ 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](https://app.codecov.io/gh/sidorares/node-mysql2/pull/938/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrey+Sidorov) | Coverage Δ | | |---|---|---| | [compression-0](https://app.codecov.io/gh/sidorares/node-mysql2/pull/938/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrey+Sidorov) | `90.12% <87.50%> (-0.22%)` | :arrow_down: | | [compression-1](https://app.codecov.io/gh/sidorares/node-mysql2/pull/938/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrey+Sidorov) | `90.31% <87.50%> (-0.02%)` | :arrow_down: | | [tls-0](https://app.codecov.io/gh/sidorares/node-mysql2/pull/938/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrey+Sidorov) | `89.84% <87.50%> (-0.02%)` | :arrow_down: | | [tls-1](https://app.codecov.io/gh/sidorares/node-mysql2/pull/938/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrey+Sidorov) | `90.12% <87.50%> (-0.02%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrey+Sidorov#carryforward-flags-in-the-pull-request-comment) to find out more.

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

jafl commented 6 months ago

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