perf: defer Error object creation to error handlers in promise wrappers - #4257

Codecov Report

❌ Patch coverage is 45.45455% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.86%. Comparing base (0b750e0) to head (a1711f7).
⚠️ Report is 3 commits behind head on master.

@@            Coverage Diff             @@
##           master    #4257      +/-   ##
==========================================
- Coverage   90.87%   90.86%   -0.01%     
==========================================
  Files          89       89              
  Lines       14498    14487      -11     
  Branches     1862     1862              
==========================================
- Hits        13175    13164      -11     
  Misses       1323     1323              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Would it be possible to communicate the caller (application) filename / line-number where the error was triggered?

It has become much harder to debug MySQL errors without knowing where in the application code they originated.

Would it be possible to communicate the caller (application) filename / line-number where the error was triggered?

It has become much harder to debug MySQL errors without knowing where in the application code they originated.

This was the original intent of manual error construction, and this PR should not change this mostly

Could you help testing this @chrisveness ? Create a simple repro case, and compare stack trace before / after this version

Also maybe we need to reevaluate if we still have to do this manually - v8 captures async stack traces manually, maybe we can rely on that

Create file bad-query.js:

import mysql from 'mysql2/promise';

const connection = await mysql.createConnection({
  host:     'localhost',
  user:     'user',
  database: 'db',
  password: 'pw',
});

const sql = 'SELECT nosuchcolumn FROM information_schema.tables';
const [ results, fields ] = await connection.query(sql);
console.log(results);
await connection.end();

Run:

$ npm install mysql2@3.21.1
$ node bad-query.js

Gives:

.../mysql2-4257/node_modules/mysql2/lib/promise/connection.js:29
    const localErr = new Error();
                     ^

Error: Unknown column 'nosuchcolumn' in 'field list'
    at PromiseConnection.query (.../mysql2-4257/node_modules/mysql2/lib/promise/connection.js:29:22)
    at file://.../mysql2-4257/bad-query.js:11:46
    at process.processTicksAndRejections (node:internal/process/task_queues:103:5) {
  code: 'ER_BAD_FIELD_ERROR',
  errno: 1054,
  sql: 'SELECT nosuchcolumn FROM information_schema.tables',
  sqlState: '42S22',
  sqlMessage: "Unknown column 'nosuchcolumn' in 'field list'"
}

Then run:

$ npm install mysql2@3.22.0
$ node bad-query.js

Gives:

.../mysql2-4257/node_modules/mysql2/lib/promise/make_done_cb.js:6
      const localErr = new Error();
                       ^

Error: Unknown column 'nosuchcolumn' in 'field list'
    at Query.onResult (.../mysql2-4257/node_modules/mysql2/lib/promise/make_done_cb.js:6:24)
    at Query.execute (.../mysql2-4257/node_modules/mysql2/lib/commands/command.js:36:14)
    at Connection.handlePacket (.../mysql2-4257/node_modules/mysql2/lib/base/connection.js:552:34)
    at PacketParser.onPacket (.../mysql2-4257/node_modules/mysql2/lib/base/connection.js:102:12)
    at PacketParser.executeStart (.../mysql2-4257/node_modules/mysql2/lib/packet_parser.js:75:16)
    at Socket.<anonymous> (.../mysql2-4257/node_modules/mysql2/lib/base/connection.js:109:25)
    at Socket.emit (node:events:508:28)
    at addChunk (node:internal/streams/readable:559:12)
    at readableAddChunkPushByteMode (node:internal/streams/readable:510:3)
    at Readable.push (node:internal/streams/readable:390:5) {
  code: 'ER_BAD_FIELD_ERROR',
  errno: 1054,
  sql: 'SELECT nosuchcolumn FROM information_schema.tables',
  sqlState: '42S22',
  sqlMessage: "Unknown column 'nosuchcolumn' in 'field list'"
}

... which has no reference to bad-query.js line number.

(Sometimes it might be straightforward to grep for the SQL string, but not always).

yeah, I should have reviewed this a bit more carefully
when we moved stack capture inside callback it broke original funcionality of capturing command stack

I'll see current state of native async stack traces but might actually revert this change if there's no better option

We could have this optional with the flag, but I'm not sure how big of a perf impact are we talking realistically compared to a full round trip of an average query. If it's really small flag not worth it and I'll just revert

I think it's best to revert.

Alternatives considered:

'lightweight stacks' -

  const obj = {};
  Error.captureStackTrace(obj, captureCallSite);

Same performance as new Error() in node, in Bun new Error() is actually faster compared to Error.captureStackTrace

native async stack traces - only captures await boundary.

@chrisveness reverted, releasing as v3.22.1