WiseLibs / better-sqlite3

The fastest and simplest library for SQLite3 in Node.js.
MIT License
5.37k stars 394 forks source link

Fix db::serialize() crashing with Electron #1036

Closed DamienEspitallier closed 1 year ago

DamienEspitallier commented 1 year ago

This is a fix for #981

It use a #ifdef encapsulation to only use the copy method on electron build.

This is the first time I use lzz and I did not achieve to output #ifndef/else/endif clause from the database.lzz file, so I use the macro.lzz instead. I hope this is acceptable :).

JoshuaWise commented 1 year ago

thanks for this fix!

Prinzhorn commented 1 month ago

@DamienEspitallier @JoshuaWise @mceachen isn't the if/else backwards? Reversing it fixes #1223 and the original #981

For repro I still use the minimal:

const Database = require('better-sqlite3');
const db = new Database(':memory:');

db.exec('CREATE TABLE foo (id)');

console.log(db.serialize());
Prinzhorn commented 1 month ago

Or maybe it is the wrong flag now? Turns out it actually works with 8.6.0 but for 11.1.2 it does not. I'm confused.

Edit: Can confirm that better-sqlite3@11.1.2 works with electron@29.4.3 but breaks with electron@30.0.0-alpha.1. But compiling it with the following changes makes it work:

image

however, this does not work (basically the original code):

image

but this works:

image

Someone smarter and more patient than me pls explain. Did they invert the flag on us?

Looking through https://grep.app/search?q=V8_COMPRESS_POINTERS_IN_SHARED_CAGE this is also super rarely used so maybe nobody noticed?

NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED is probably the appropriate flag and we were just lucky that it previously aligned with V8_COMPRESS_POINTERS_IN_SHARED_CAGE

TheOneTheOnlyJJ commented 1 month ago

Can a fix for this be expected in the near future?