From be9c94f6466cdaf307990f204f61c889de65d27b Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Fri, 9 Feb 2018 13:39:30 +0100 Subject: [PATCH 1/3] remove location after destroy() because windows can't in C++ land --- leveldown.js | 27 +++++++++++++++++++++++---- test/destroy-test.js | 43 ++++++++++++++++++++++++++++++++++++++----- 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/leveldown.js b/leveldown.js index 1d3cdb3de94..8edb65c3083 100644 --- a/leveldown.js +++ b/leveldown.js @@ -1,11 +1,9 @@ const util = require('util') , AbstractLevelDOWN = require('abstract-leveldown').AbstractLevelDOWN - , binding = require('bindings')('leveldown').leveldown - , ChainedBatch = require('./chained-batch') , Iterator = require('./iterator') - + , fs = require('fs') function LevelDOWN (location) { if (!(this instanceof LevelDOWN)) @@ -100,7 +98,28 @@ LevelDOWN.destroy = function (location, callback) { if (typeof callback != 'function') throw new Error('destroy() requires a callback function argument') - binding.destroy(location, callback) + binding.destroy(location, function (err) { + if (err) return callback(err) + + // On Windows, RocksDB silently fails to remove the directory because its + // Logger, which is instantiated on destroy(), has an open file handle on a + // LOG file. Destroy() removes this file but Windows won't actually delete + // it until the handle is released. This happens when destroy() goes out of + // scope, which disposes the Logger. So back in JS-land, we can again + // attempt to remove the directory. This is merely a workaround because + // arguably RocksDB should not instantiate a Logger or open a file at all. + fs.rmdir(location, function (err) { + if (err) { + // Ignore this error in case there are non-RocksDB files left. + if (err.code === 'ENOTEMPTY') return callback() + if (err.code === 'ENOENT') return callback() + + return callback(err) + } + + callback() + }) + }) } diff --git a/test/destroy-test.js b/test/destroy-test.js index 8cfaee33ff9..b55b39011c5 100644 --- a/test/destroy-test.js +++ b/test/destroy-test.js @@ -1,8 +1,10 @@ const test = require('tape') + , testCommon = require('abstract-leveldown/testCommon') , fs = require('fs') , path = require('path') , mkfiletree = require('mkfiletree') , readfiletree = require('readfiletree') + , rimraf = require('rimraf') , leveldown = require('../') , makeTest = require('./make') @@ -24,11 +26,42 @@ test('test callback-less, 1-arg, destroy() throws', function (t) { t.end() }) -test('test destroy non-existant directory', function (t) { - leveldown.destroy('/1/2/3/4', function () { - t.equal(arguments.length, 1, 'error object returned on callback') - t.equal(/.*IO error.*\/1\/2\/3\/4\/LOCK.*/.test(arguments[0]), true) - t.end() +test('test destroy non-existent directory', function (t) { + t.plan(4) + + var location = testCommon.location() + var parent = path.dirname(location) + + // For symmetry with the opposite test below. + t.ok(fs.existsSync(parent), 'parent exists before') + + // Cleanup to avoid conflicts with other tests + rimraf(location, { glob: false }, function (err) { + t.ifError(err, 'no rimraf error') + + leveldown.destroy(location, function () { + t.is(arguments.length, 0, 'no arguments returned on callback') + + // Assert that destroy() didn't inadvertently create the directory. + // Or if it did, that it was at least cleaned up afterwards. + t.notOk(fs.existsSync(location), 'directory does not exist after') + }) + }) +}) + +test('test destroy non-existent parent directory', function (t) { + t.plan(4) + + var location = '/1/2/3/4' + var parent = path.dirname(location) + + t.notOk(fs.existsSync(parent), 'parent does not exist before') + + leveldown.destroy(location, function () { + // This behavior differs from leveldown, which is silent. + t.is(arguments.length, 1, 'error object returned on callback') + t.ok(/.*IO error.*\/1\/2\/3\/4\/LOCK.*/.test(arguments[0]), 'got IO error') + t.notOk(fs.existsSync(location), 'directory does not exist after') }) }) From fba4913cadd97d306302a04c253448be46aea29c Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Fri, 9 Feb 2018 13:40:46 +0100 Subject: [PATCH 2/3] handle all errors in destroy tests --- test/destroy-test.js | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/test/destroy-test.js b/test/destroy-test.js index b55b39011c5..57340ce30a9 100644 --- a/test/destroy-test.js +++ b/test/destroy-test.js @@ -70,15 +70,19 @@ test('test destroy non leveldb directory', function (t) { 'foo': 'FOO' , 'bar': { 'one': 'ONE', 'two': 'TWO', 'three': 'THREE' } } + mkfiletree.makeTemp('destroy-test', tree, function (err, dir) { - t.notOk(err, 'no error') - leveldown.destroy(dir, function () { - t.ok(arguments, 'no arguments') + t.ifError(err, 'no close error') + + leveldown.destroy(dir, function (err) { + t.ifError(err, 'no destroy error') + readfiletree(dir, function (err, actual) { - t.notOk(err, 'no error') + t.ifError(err, 'no read error') t.deepEqual(actual, tree, 'directory remains untouched') + mkfiletree.cleanUp(function (err) { - t.notOk(err, 'no error') + t.ifError(err, 'no cleanup error') t.end() }) }) @@ -88,9 +92,12 @@ test('test destroy non leveldb directory', function (t) { makeTest('test destroy() cleans and removes leveldb-only dir', function (db, t, done, location) { db.close(function (err) { - t.notOk(err, 'no error') - leveldown.destroy(location, function () { + t.ifError(err, 'no close error') + + leveldown.destroy(location, function (err) { + t.ifError(err, 'no destroy error') t.notOk(fs.existsSync(location), 'directory completely removed') + done(false) }) }) @@ -98,11 +105,17 @@ makeTest('test destroy() cleans and removes leveldb-only dir', function (db, t, makeTest('test destroy() cleans and removes only leveldb parts of a dir', function (db, t, done, location) { fs.writeFileSync(path.join(location, 'foo'), 'FOO') + db.close(function (err) { - t.notOk(err, 'no error') - leveldown.destroy(location, function () { + t.ifError(err, 'no close error') + + leveldown.destroy(location, function (err) { + t.ifError(err, 'no destroy error') + readfiletree(location, function (err, tree) { + t.ifError(err, 'no read error') t.deepEqual(tree, { 'foo': 'FOO' }, 'non-leveldb files left intact') + done(false) }) }) From 028f4f5788747ba73eac155bf12c1ae6d13f8b83 Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Fri, 9 Feb 2018 13:45:30 +0100 Subject: [PATCH 3/3] remove console.log() from approximate-size-test --- test/approximate-size-test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/approximate-size-test.js b/test/approximate-size-test.js index 0e662d6408d..d400cfa63fb 100644 --- a/test/approximate-size-test.js +++ b/test/approximate-size-test.js @@ -8,7 +8,6 @@ test('setUp common for approximate size', testCommon.setUp) test('setUp db', function (t) { db = leveldown(testCommon.location()) - console.log('db', db) db.open(t.end.bind(t)) })