diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a647deb8..0d6c9910 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -36,3 +36,5 @@ jobs: uses: GabrielBB/xvfb-action@v1 with: run: npm run test-electron + - name: Test GC + run: npm run test-gc diff --git a/binding.cc b/binding.cc index 6a1badbf..4777609a 100644 --- a/binding.cc +++ b/binding.cc @@ -373,9 +373,14 @@ struct Database { db_(NULL), currentIteratorId_(0), pendingCloseWorker_(NULL), + ref_(NULL), priorityWork_(0) {} ~Database () { + if (ref_ != NULL) { + napi_delete_reference(env_, ref_); + } + if (db_ != NULL) { delete db_; db_ = NULL; @@ -458,11 +463,13 @@ struct Database { } void IncrementPriorityWork () { - ++priorityWork_; + napi_reference_ref(env_, ref_, &priorityWork_); } void DecrementPriorityWork () { - if (--priorityWork_ == 0 && pendingCloseWorker_ != NULL) { + napi_reference_unref(env_, ref_, &priorityWork_); + + if (priorityWork_ == 0 && pendingCloseWorker_ != NULL) { pendingCloseWorker_->Queue(); pendingCloseWorker_ = NULL; } @@ -477,6 +484,7 @@ struct Database { uint32_t currentIteratorId_; BaseWorker *pendingCloseWorker_; std::map< uint32_t, Iterator * > iterators_; + napi_ref ref_; private: uint32_t priorityWork_; @@ -841,11 +849,16 @@ NAPI_METHOD(db_init) { NAPI_STATUS_THROWS(napi_create_external(env, database, FinalizeDatabase, NULL, &result)); + + // Reference counter to prevent GC of database while priority workers are active + NAPI_STATUS_THROWS(napi_create_reference(env, result, 0, &database->ref_)); + return result; } /** * Worker class for opening a database. + * TODO: shouldn't this be a PriorityWorker? */ struct OpenWorker final : public BaseWorker { OpenWorker (napi_env env, @@ -1185,7 +1198,6 @@ struct ClearWorker final : public PriorityWorker { } ~ClearWorker () { - // TODO: write GC tests delete baseIterator_; delete writeOptions_; } @@ -1538,6 +1550,7 @@ struct EndWorker final : public BaseWorker { } void HandleOKCallback () override { + // TODO: if we don't use EndWorker, do we still delete the reference? napi_delete_reference(env_, iterator_->Detach()); BaseWorker::HandleOKCallback(); } diff --git a/test/clear-gc-test.js b/test/clear-gc-test.js new file mode 100644 index 00000000..2794878a --- /dev/null +++ b/test/clear-gc-test.js @@ -0,0 +1,47 @@ +'use strict' + +const test = require('tape') +const testCommon = require('./common') +const sourceData = [] + +for (let i = 0; i < 1e3; i++) { + sourceData.push({ + type: 'put', + key: i.toString(), + value: Math.random().toString() + }) +} + +test('db without ref does not get GCed while clear() is in progress', function (t) { + t.plan(4) + + let db = testCommon.factory() + + db.open(function (err) { + t.ifError(err, 'no open error') + + // Insert test data + db.batch(sourceData.slice(), function (err) { + t.ifError(err, 'no batch error') + + // Start async work + db.clear(function () { + t.pass('got callback') + + // Give GC another chance to run, to rule out other issues. + setImmediate(function () { + if (global.gc) global.gc() + t.pass() + }) + }) + + // Remove reference. The db should not get garbage collected + // until after the clear() callback, thanks to a napi_ref. + db = null + + // Useful for manual testing with "node --expose-gc". + // The pending tap assertion may also allow GC to kick in. + if (global.gc) global.gc() + }) + }) +}) diff --git a/test/gc.js b/test/gc.js index a5655f91..956cc2ad 100644 --- a/test/gc.js +++ b/test/gc.js @@ -10,3 +10,4 @@ if (!global.gc) { require('./cleanup-hanging-iterators-test') require('./iterator-gc-test') require('./chained-batch-gc-test') +require('./clear-gc-test')