Merge pull request facebook#35 from Level/fix-windows-destroy

Fix destroy() on Windows

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()
+    })
+  })
 }
 
 

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))
 })
 

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')
   })
 })
 
@@ -37,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()
         })
       })
@@ -55,21 +92,30 @@ 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)
     })
   })
 })
 
 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)
       })
     })