diff --git a/binding.cc b/binding.cc index 577d28d8..0472d1f5 100644 --- a/binding.cc +++ b/binding.cc @@ -454,7 +454,6 @@ static void FinalizeDatabase (napi_env env, void* data, void* hint) { /** * Base worker class for doing async work that defers closing the database. - * @TODO Use this for Get-, Del-, Batch-, BatchWrite-, ApproximateSize- and CompactRangeWorker too. */ struct PriorityWorker : public BaseWorker { PriorityWorker (napi_env env, Database* database, napi_value callback, const char* resourceName) @@ -929,14 +928,14 @@ NAPI_METHOD(db_put) { /** * Worker class for getting a value from a database. */ -struct GetWorker final : public BaseWorker { +struct GetWorker final : public PriorityWorker { GetWorker (napi_env env, Database* database, napi_value callback, leveldb::Slice key, bool asBuffer, bool fillCache) - : BaseWorker(env, database, callback, "leveldown.db.get"), + : PriorityWorker(env, database, callback, "leveldown.db.get"), key_(key), asBuffer_(asBuffer) { options_.fill_cache = fillCache; @@ -994,13 +993,13 @@ NAPI_METHOD(db_get) { /** * Worker class for deleting a value from a database. */ -struct DelWorker final : public BaseWorker { +struct DelWorker final : public PriorityWorker { DelWorker (napi_env env, Database* database, napi_value callback, leveldb::Slice key, bool sync) - : BaseWorker(env, database, callback, "leveldown.db.del"), + : PriorityWorker(env, database, callback, "leveldown.db.del"), key_(key) { options_.sync = sync; } @@ -1037,13 +1036,13 @@ NAPI_METHOD(db_del) { /** * Worker class for calculating the size of a range. */ -struct ApproximateSizeWorker final : public BaseWorker { +struct ApproximateSizeWorker final : public PriorityWorker { ApproximateSizeWorker (napi_env env, Database* database, napi_value callback, leveldb::Slice start, leveldb::Slice end) - : BaseWorker(env, database, callback, "leveldown.db.approximate_size"), + : PriorityWorker(env, database, callback, "leveldown.db.approximate_size"), start_(start), end_(end) {} ~ApproximateSizeWorker () { @@ -1093,13 +1092,13 @@ NAPI_METHOD(db_approximate_size) { /** * Worker class for compacting a range in a database. */ -struct CompactRangeWorker final : public BaseWorker { +struct CompactRangeWorker final : public PriorityWorker { CompactRangeWorker (napi_env env, Database* database, napi_value callback, leveldb::Slice start, leveldb::Slice end) - : BaseWorker(env, database, callback, "leveldown.db.compact_range"), + : PriorityWorker(env, database, callback, "leveldown.db.compact_range"), start_(start), end_(end) {} ~CompactRangeWorker () { @@ -1579,13 +1578,13 @@ NAPI_METHOD(iterator_next) { /** * Worker class for batch write operation. */ -struct BatchWorker final : public BaseWorker { +struct BatchWorker final : public PriorityWorker { BatchWorker (napi_env env, Database* database, napi_value callback, leveldb::WriteBatch* batch, bool sync) - : BaseWorker(env, database, callback, "leveldown.batch.do"), + : PriorityWorker(env, database, callback, "leveldown.batch.do"), batch_(batch) { options_.sync = sync; } @@ -1772,12 +1771,12 @@ NAPI_METHOD(batch_clear) { /** * Worker class for batch write operation. */ -struct BatchWriteWorker final : public BaseWorker { +struct BatchWriteWorker final : public PriorityWorker { BatchWriteWorker (napi_env env, Batch* batch, napi_value callback, bool sync) - : BaseWorker(env, batch->database_, callback, "leveldown.batch.write"), + : PriorityWorker(env, batch->database_, callback, "leveldown.batch.write"), batch_(batch), sync_(sync) {} diff --git a/test/segfault-test.js b/test/segfault-test.js index f0df413f..e7941520 100644 --- a/test/segfault-test.js +++ b/test/segfault-test.js @@ -1,28 +1,89 @@ const test = require('tape') const testCommon = require('./common') +const operations = [] -// See https://github.com/Level/leveldown/issues/157 -test('close() does not segfault if there is a pending write', function (t) { - t.plan(3) +// The db must wait for pending operations to finish before closing. This to +// prevent segfaults and in the case of compactRange() to prevent hanging. See +// https://github.com/Level/leveldown/issues/157 and 32. +function testPending (name, expectedCount, fn) { + operations.push(fn) - const db = testCommon.factory() + test(`close() waits for pending ${name}`, function (t) { + const db = testCommon.factory() + let count = 0 - db.open(function (err) { - t.ifError(err, 'no open error') + db.open(function (err) { + t.ifError(err, 'no error from open()') - // The "sync" option seems to be a reliable way to trigger a segfault, - // but is not necessarily the cause of that segfault. More likely, it - // exposes a race condition that's already there. - db.put('foo', 'bar', { sync: true }, function (err) { - // We never get here, due to segfault. - t.ifError(err, 'no put error') - }) + db.put('key', 'value', function (err) { + t.ifError(err, 'no error from put()') - db.close(function (err) { - // We never get here, due to segfault. - t.ifError(err, 'no close error') + fn(db, function (err) { + count++ + t.ifError(err, 'no error from operation') + }) + + db.close(function (err) { + t.ifError(err, 'no error from close()') + t.is(count, expectedCount, 'operation(s) finished before close') + t.end() + }) + }) }) }) +} + +testPending('get()', 1, function (db, next) { + db.get('key', next) +}) + +testPending('put()', 1, function (db, next) { + db.put('key2', 'value', next) +}) + +testPending('put() with { sync }', 1, function (db, next) { + // The sync option makes the operation slower and thus more likely to + // cause a segfault (if closing were to happen during the operation). + db.put('key2', 'value', { sync: true }, next) +}) + +testPending('del()', 1, function (db, next) { + db.del('key', next) +}) + +testPending('del() with { sync }', 1, function (db, next) { + db.del('key', { sync: true }, next) +}) + +testPending('batch([])', 1, function (db, next) { + db.batch([{ type: 'del', key: 'key' }], next) +}) + +testPending('batch([]) with { sync }', 1, function (db, next) { + db.batch([{ type: 'del', key: 'key' }], { sync: true }, next) +}) + +testPending('batch()', 1, function (db, next) { + db.batch().del('key').write(next) +}) + +testPending('batch() with { sync }', 1, function (db, next) { + db.batch().del('key').write({ sync: true }, next) +}) + +testPending('approximateSize()', 1, function (db, next) { + db.approximateSize('a', 'z', next) +}) + +testPending('compactRange()', 1, function (db, next) { + db.compactRange('a', 'z', next) +}) + +// Test multiple pending operations, using all of the above. +testPending('operations', operations.length, function (db, next) { + for (let fn of operations.slice(0, -1)) { + fn(db, next) + } }) // See https://github.com/Level/leveldown/issues/134