diff --git a/binding.cc b/binding.cc index 0472d1f5..13a36070 100644 --- a/binding.cc +++ b/binding.cc @@ -1773,14 +1773,20 @@ NAPI_METHOD(batch_clear) { */ struct BatchWriteWorker final : public PriorityWorker { BatchWriteWorker (napi_env env, + napi_value context, Batch* batch, napi_value callback, bool sync) : PriorityWorker(env, batch->database_, callback, "leveldown.batch.write"), batch_(batch), - sync_(sync) {} + sync_(sync) { + // Prevent GC of batch object before we execute + NAPI_STATUS_THROWS(napi_create_reference(env_, context, 1, &contextRef_)); + } - ~BatchWriteWorker () {} + ~BatchWriteWorker () { + napi_delete_reference(env_, contextRef_); + } void DoExecute () override { SetStatus(batch_->Write(sync_)); @@ -1788,6 +1794,9 @@ struct BatchWriteWorker final : public PriorityWorker { Batch* batch_; bool sync_; + +private: + napi_ref contextRef_; }; /** @@ -1801,7 +1810,7 @@ NAPI_METHOD(batch_write) { bool sync = BooleanProperty(env, options, "sync", false); napi_value callback = argv[2]; - BatchWriteWorker* worker = new BatchWriteWorker(env, batch, callback, sync); + BatchWriteWorker* worker = new BatchWriteWorker(env, argv[0], batch, callback, sync); worker->Queue(); NAPI_RETURN_UNDEFINED(); diff --git a/package.json b/package.json index 87e888cf..84344b02 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,7 @@ "scripts": { "install": "node-gyp-build", "test": "standard && nyc tape test/*-test.js", - "test-gc": "npx -n=--expose-gc tape test/{cleanup,iterator-gc}*-test.js", + "test-gc": "npx -n=--expose-gc tape test/{cleanup,iterator-gc,chained-batch-gc}*-test.js", "test-electron": "electron test/electron.js", "coverage": "nyc report --reporter=text-lcov | coveralls", "rebuild": "node-gyp rebuild", diff --git a/test/chained-batch-gc-test.js b/test/chained-batch-gc-test.js new file mode 100644 index 00000000..1981efcc --- /dev/null +++ b/test/chained-batch-gc-test.js @@ -0,0 +1,37 @@ +'use strict' + +const test = require('tape') +const testCommon = require('./common') + +// When we have a chained batch object without a reference, V8 might GC it +// before we get a chance to (asynchronously) write the batch. +test('chained batch without ref does not get GCed before write', function (t) { + t.plan(2) + + const db = testCommon.factory() + + db.open(function (err) { + t.ifError(err, 'no open error') + + let batch = db.batch() + + for (let i = 0; i < 1e3; i++) { + batch.put(String(i), 'value') + } + + // The sync option makes the operation slower and thus more likely to + // cause a segfault (if the batch were to be GC-ed before it is written). + batch.write({ sync: true }, function (err) { + t.ifError(err, 'no error from write()') + }) + + // Remove reference + batch = null + + if (global.gc) { + // This is the reliable way to trigger GC (and the bug if it exists). + // Useful for manual testing with "node --expose-gc". + global.gc() + } + }) +})