sqlite: custom function callback errors should be reported to sqlite API

[Renegade334]

In the callback wrapper for user-provided functions (ie. those added with DatabaseSync#function()), error conditions in the JS callback result in an early return to the sqlite API:

node/src/node_sqlite.cc

Lines 202 to 207 in 1c7c32f

	MaybeLocal<Value> retval =
	fn->Call(env->context(), recv, argc, js_argv.data());
	Local<Value> result;
	if (!retval.ToLocal(&result)) {
	return;
	}

In sqlite, this results in the return value of the user-defined function being implicitly set to NULL. It does not signal an error condition from the callback.

This raises the potential for user function callbacks to encounter error conditions, but still cause side-effects in sqlite. As a basic example:

const sqlite = require('node:sqlite');
const db = new sqlite.DatabaseSync(':memory:');
db.exec('CREATE TABLE test (id NUMBER NOT NULL PRIMARY KEY, data TEXT)');

// create a user-defined function that throws
db.function('MYFUNC', function () { throw new Error('MYFUNC callback threw!') });

// fill table with arbitrary data
{
  const stmt = db.prepare('INSERT INTO test (id, data) VALUES (?, ?)');
  for (let i = 0; i < 10; i++) {
    stmt.run(i, Math.random().toString());
  }
}

db.prepare('SELECT * FROM test').all();
// [
//   [Object: null prototype] { id: 0, data: '0.5786694350792139' },
//   [Object: null prototype] { id: 1, data: '0.9442753197210123' },
//   ...
// ]

db.exec('UPDATE test SET data = MYFUNC()');
// Uncaught Error: MYFUNC callback threw!

db.prepare('SELECT * FROM test').all();
// [
//   [Object: null prototype] { id: 0, data: null },
//   [Object: null prototype] { id: 1, data: null },
//   ...
// ]

Error conditions in the user function callback should call sqlite3_result_error() in order to signal the sqlite API that an error has occurred and that the query should be aborted.

[cjihrig]

Nice find. I've put together a patch in cjihrig@f113c39. There is already another PR though (#56773), so I'm not sure how we want to proceed. Even though node:sqlite is experimental, I think we should aim to get this fixed in the next release possible since it risks data corruption.

[Renegade334]

My $0.02 would be that there are considerations beyond simply adding a one-liner (I was going to comment about the other silent returns, but the point about deduplicating exceptions is a very good one) that make the best course of action to open a new PR and supersede the first. With what would likely be a lot of iterations of recommend-modify-review, I doubt the existing one would be committable in time for the current release cycle.