destroyStringParams: set owned = false after free

[kristate]

Hello -- I was hitting the following assert:

https://github.com/josephg/node-foundationdb/blob/e0006e4d28a6c42be9003e847f5063a3442b7a1f/src/napi/transaction.cpp#L72

Please let me know if this patch is the right way to go.

Thanks.

[josephg]

Sorry; this is the first I’m hearing about this bug. Can you give me more context? Do you have a test case which trips the assert?

[kristate]

Narrowed it down to this:

const fdb = require('foundationdb');
fdb.setAPIVersion(600);

const db = fdb.openSync();

const testCase = async () => {
  await db.doTransaction(async tn => {
    for await (const batch of tn.getRange( '\x00', '\x01' )) {}
  });
};
testCase();

[kristate]

Keep in mind, it doesn't matter where the null-byte is placed

const fdb = require('foundationdb');
fdb.setAPIVersion(600);

const db = fdb.openSync();

const testCase = async () => {
  await db.doTransaction(async tn => {
    for await (const batch of tn.getRange( 'hello\x00', '\x01' )) {}
  });
};
testCase();

[josephg]

Fascinating - I can't reproduce this in the unit test suite, but I can repro it from running that script directly. It looks like adding any prefix - even an empty string - makes it work correctly:

const db = fdb.openSync().at(''); // fixes the issue

[josephg]

Looked into it in more detail. You're totally right - that fixes the issue. But I don't like how obvious this is, and how the tests didn't run into this problem. The deeper issue is that we're coercing the passed key and value into buffers in almost every codepath from javascript. And as a result the "key is a string" codepath is rarely-if-ever exercised by the tests. (... because the tests always operates under a prefix.)

I think the better fix would be to make all keys passed from node -> C to be buffers. That way there's fewer codepaths to test, and I'll have more confidence in correctness. The downside is that performance will (marginally) suffer in many cases - since string -> buffer conversions are more efficient from C.

For now I'm merging this as-is with your test case added to the suite, but it might make sense to revisit this later with a better fix.

[josephg]

📦 foundationdb@0.10.4