SqliteErrors are silently swallowed inside Worker Threads

[Prinzhorn]

Node v14.16.0
better-sqlite3 7.1.1

index.js

const path = require('path');
const { Worker } = require('worker_threads');

let worker = new Worker(path.join(__dirname, 'worker.js'));

worker
  .on('message', (message) => {
    console.log(message);
  })
  .on('error', (err) => {
    console.error('Worker error:');
    console.error(err);
  })
  .on('exit', (code) => {
    console.log(`server worker exited with code ${code}`);
  });

worker.js

const { parentPort } = require('worker_threads');
const Database = require('better-sqlite3');
const { SqliteError } = require('better-sqlite3');

const db = new Database(':memory:');

parentPort.postMessage('Hello from worker');

throw new SqliteError('nope', 'nope');
// or sth. like db.prepare('nope');
// If you comment out the above line and instead use a regular Error the parent process gets a propert 'error'.
throw new Error('yes');

Steps

1. node index.js
2. The worker crashes but the error is only an empty object {} which is easy to miss in logs

Hello from worker
Worker error:
{}
server worker exited with code 1

Throwing a regular error results in this:

Hello from worker
Worker error:
Error: yes
    at Object.<anonymous> (/home/alex/src/worker-error/worker.js:13:7)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.Module._load (internal/modules/cjs/loader.js:769:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at MessagePort.<anonymous> (internal/main/worker_thread.js:174:24)
    at MessagePort.[nodejs.internal.kHybridDispatch] (internal/event_target.js:354:41)
    at MessagePort.exports.emitMessage (internal/per_context/messageport.js:18:26)
server worker exited with code 1

[Prinzhorn]

The worker demo from the docs shows the same behavior and prints a generic object

{}
worker exited with code 1
(node:43396) UnhandledPromiseRejectionWarning: #<Object>

[Prinzhorn]

I originally thought this happened because SqliteError is not Error and the structured cloning algorithm wouldn't properly serialized it across the thread boundary. But now I don't know what's causing it and I need to look at it again later.

[Prinzhorn]

This can also be reproduced by simply doing postMessage(new SqliteError('a', 'b')). The reason the object is empty is because all properties are defined as enumerable: false. So far I haven't been able to actually get any custom Error object transferred, regardless of which pattern for inheritance is used (which kind of makes sense, because the other thread might not even have that class available). I must assume that this is a limitation of Worker Threads themselves and their error handling seems to have some other issues nodejs/node#35506

If we cannot make SqliteError appear as Error in the main thread we still want some information to make it across. I don't know why you went with enumerable: false and maybe that's all we need to solve this.

Personally I will now handle uncaughtException in the worker and manually downgrade SqliteError to Error and rethrow it.

[Prinzhorn]

nodejs/node#37988

[JoshuaWise]

I can't call this a bug with better-sqlite3 since it's a behavior of Node.js itself, but I do acknowledge that it would be nice if transferring custom errors between threads were possible. I'll keep my eye on this.

[Prinzhorn]

I can't call this a bug with better-sqlite3 since it's a behavior of Node.js itself

I agree about 51%. But enumerable: false makes the current situation worse. Other custom Error classes at least give some information.

Would you be opposed to a PR that adds an option to only throw native Error? I don't like the idea very much, but I'm still lacking a workaround. Using uncaughtException did not work as I hoped.

[JoshuaWise]

The reason for enumerable: false is that all existing properties on native Error objects are enumerable: false, so I think it's worth adhering to that pattern so I don't surprise people who might be using Object.keys() or Object.assign() on error objects while expecting a certain behavior that they're used to.

Can't you just write a try-catch block around your worker code, detect SqliteError there, and then send a message to the parent thread with your desired information?

[Prinzhorn]

The reason for enumerable: false is that all existing properties on native Error objects are enumerable: false, so I think it's worth adhering to that pattern so I don't surprise people who might be using Object.keys() or Object.assign() on error objects while expecting a certain behavior that they're used to.

I see

Can't you just write a try-catch block around your worker code, detect SqliteError there, and then send a message to the parent thread with your desired information?

I would like to keep the simplicity of the error event and not handle errors using worker message. The worker API is already awkward enough.
But even if I wanted to, this sounds much easier than you think. The whole application (fastify HTTP/WebSocket server) lives in a worker thread (spawned from the Electron main thread). Additionally some routes spawn piscina workers as well. Those are classic workers though where I could definitely rethrow a native Error inside the catch block since these workers run user provided queries which will naturally cause SqliteErrors. But the main application has SQL in many different places and also builds some queries with raw strings before preparing them (there's some complex grid-filtering going on where the user can filter on many columns so the queries are dynamic).

Edit: Also if I wanted to make a custom error logic using message event, then I have a race between the postMessage call and process.exit() because I still want the worker go down (ỳou shouldn't keep an application running after catching uncaughtException event). And if I wait for an answer from the main thread to acknowledge my error and then asynchronously process.exit() I keep the thread running in an inconsistent state for this period.

[JoshuaWise]

This worked for me:

master.js

const { Worker } = require('worker_threads');
const worker = new Worker('./worker.js');

worker
	.on('online', () => {
		console.log('online');
	})
	.on('error', (err) => {
		console.log('error: %s', err);
	})
	.on('exit', (code) => {
		console.log('exit: %s', code);
		process.exit();
	});

worker.js

const { SqliteError } = require('better-sqlite3');

function propagateThreadError(err) {
	if (err instanceof SqliteError) {
		const nativeErr = new Error(err.message);
		nativeErr.isSqliteError = true;
		nativeErr.code = err.code;
		nativeErr.stack = err.stack;
		err = nativeErr;
	}
	throw err;
}

process.on('uncaughtException', propagateThreadError);
process.on('unhandledRejection', propagateThreadError);

// These wrapper functions are just to demonstrate that the stack trace is preserved.
(function foo() {
	(function bar() {
		throw new SqliteError('oops!', 'FAKE_CODE');
	})();
})();

[Prinzhorn]

OMG I found the problem. And maybe it's a bug in Node.js.

The code you've posted is basically identical to what I've tried and it didn't work. I just tracked this down and the exit code is 0. Similar to your worker example I only look at workerError when code !== 0. So basically throwing inside uncaughtException causes an error event but the worker exits with code 0. I'll report this to node and see what happens.

So I guess we can consider this solved. I might enhance your code by using v8.deserialize(v8.serialize(err)) instanceof Error to catch anything that is not an error or cannot be serialized to one.

[Prinzhorn]

nodejs/node#37996