seald / nedb

The JavaScript Database, for Node.js, nw.js, electron and the browser
MIT License
351 stars 32 forks source link

EPERM Exception when datastore is at the root of a disk on Windows #48

Closed Type8 closed 10 months ago

Type8 commented 10 months ago

Hi, I'm using NeDB in a custom NodeJS application that I maintain since 2018 and I'm still enjoying it. I'm trying now to move to this fork.

In my use case the filename is in an external disk (USB or SD) and the target OS is Windows. In the @seald-io/nedb implementation the load of the datastore throws an exception. Here a sample:

$ pwd
/c/Users/user/Desktop/tmp/seald

$ cat index.js
const Datastore = require("@seald-io/nedb");

try {
    // E:/ exists and I have RW permission
    const db = new Datastore({ filename: 'E:/tmp.datastore', autoload: true});
}
catch (err) {
    console.err(err);
}
$ node index.js

node:internal/process/promises:288
            triggerUncaughtException(err, true /* fromPromise */);
            ^

[Error: EPERM: operation not permitted, mkdir 'E:\'] {
  errno: -4048,
  code: 'EPERM',
  syscall: 'mkdir',
  path: 'E:\\'
}

I tried different version of the library and this is what I got:

Versions <= 2.x -> it works Versions >= 3.x -> it throws EPERM exception

Node: 18.18.2 OS: windows

Is this a regression? Thanks!

Umberto

tex0l commented 10 months ago

I admit this is not something I tested specifically, it might very well be a regression, the storage module has been rewritten completely with the promise based API of Node.js, there might be a difference between the two.

As I don't have a Windows environment at my disposal, could you change your code to a manual call to loadDatabaseAsync instead of relying on the autoload: true, and give me the stack trace please?

tex0l commented 10 months ago

After a quick look, there might be something breaking: we introduced in 3.0.0 file / dir modes management, which should not be used on Windows as per Node.js documentation, but may very well be.

The modes NeDB now uses are 0o644 for files and 0o755 for directories, but Node's default is 0o666 and 0o777.

Could you set modes: { fileMode: 0o666, dirMode: 0o777 } in the options to see if it works?

tex0l commented 10 months ago

Ok I think I got it:

basically, what seems to fails is the ensureDirectoryExistsAsync which executes fs.promises.mkdir('E:\', { recursive: true, mode: 0o755 }).

previously (before 3.0.0) it was fs.mkdir('E:\', { recursive: true }, callback)

The two differences are:

  1. the mode was not settable before, we had to implement it, but I checked in libuv which is used by Node.js to perform the mkdir call, the argument is just ignored, it cannot be the mode.
  2. the error management

Previously, the error were just ignored, see the implementation in v2.2.2 https://github.com/seald/nedb/blob/cd57534c906ca6c267f6c9f0d8282f58b4eb9ade/lib/persistence.js#L245-L305

But in the reimplementation introduced in 3.0.0, I now catch the errors : https://github.com/seald/nedb/blob/07d0802b1df2d3db20fb201d799957f7714edeb9/lib/persistence.js#L303-L336

I strongly believe it worked previously because of a mismanagement of the errors.

I think the same error would happen if we were to mkdir / on non-windows environements.

Another test which might be useful to debug this out, could you instantiate the Datastore in a subdirectory of E:\ like, E:\subDir\tmp.database.

Type8 commented 10 months ago

Wow, with a subdir it works! no need to change modes :)

const db = new Datastore({ filename: 'E:/subDir/tmp.datastore' });

(async _ => {
    await db.loadDatabaseAsync();
})();

Do you think is it possible to support datastores in the root of the disks (like E:\datastore.db)?

tex0l commented 10 months ago

Ok great, I think it is really the fact that you are at the root of the drive on Windows, and an mkdirp on it fails contrary to other OSes (I tested on Fedora, it works, on macOS it fails but with SIP even as root I cannot write to /test).

Basically, if in ensureDirectoryExistsAsync I check that the directory I want to mkdirp on is the root directory and just test a stat on it in this case instead of mkdirp, I believe it is feasible, however without a test bench it is hard to make sure, other things may break.

I'll try to release a preversion after testing with a colleague who has a windows env.

tex0l commented 10 months ago

Capture d’écran 2023-12-11 aΜ€ 18 07 37

RTFM as they say... RTFM... πŸ˜‚ sorry about this!

Type8 commented 10 months ago

Good :D

Thanks for the support. If you need further tests, let me know :)

tex0l commented 10 months ago

@Type8 Can you please check that v4.0.3-0 works?

Type8 commented 10 months ago

Hi :) Actually i think something strange is happening. Here a quick example:

const Datastore = require("@seald-io/nedb");

const rootDatastore = new Datastore({ filename: 'E:/tmp.datastore' });
const subDirDatastore = new Datastore({ filename: 'E:/subdir/tmp.datastore' });

(async _ => {

    await rootDatastore.loadDatabaseAsync();
    await subDirDatastore.loadDatabaseAsync();

})();

The output is:

$ node  index.js 
node:internal/process/promises:289
            triggerUncaughtException(err, true /* fromPromise */);
            ^

[Error: ENOENT: no such file or directory, open 'E:\subdir\tmp.datastore'] {
  errno: -4058,
  code: 'ENOENT',
  syscall: 'open',
  path: 'E:\\subdir\\tmp.datastore'
}

@seald-io/nedb version:

  "dependencies": {
    "@seald-io/nedb": "^4.0.3-0"
  }

The line that throws is the

    await subDirDatastore.loadDatabaseAsync();

EDIT: the loadDatabaseAsync() method does not create the subdir and the datastore file in the filesystem

tex0l commented 10 months ago

Ok, that's not right, thanks. I'll have to get hold of a windows env to make deeper tests.

Type8 commented 10 months ago

Fun fact: this example works!

const Datastore = require("@seald-io/nedb");

const rootDatastore = new Datastore({ filename: 'E:/tmp.datastore' });
const subDirDatastore = new Datastore({ filename: 'E:/subdir/anothersubdir/tmp.datastore' });

(async _ => {

    await rootDatastore.loadDatabaseAsync();
    await subDirDatastore.loadDatabaseAsync();

})();

I suppose the error is in how path.parse splits the path into { root, dir, base, ext, name }.

IF the datastore path has only one subDir, the method ensureDirectoryExistsAsync is called twice:

so the mkdirAsync is never called.

When there is more than one subdir, the ensureDirectoryExistsAsync is called twice but the path.parse acts slighly differenty:

With this object, the mkdirAsync is called correctly :)

Type8 commented 10 months ago

Sorry, the ensureDirectoryExistsAsync run twice in my example because i am running the above example, where i'm working with 2 datatores.

Anyway i still think the problem is that path.parse does not react as we expect...

dir: E:/subdir/anothersubdir  ===> parsedDir:  {
  root: 'E:\\',
  dir: 'E:\\subdir',
  base: 'anothersubdir',
  ext: '',
  name: 'anothersubdir'
}
dir: E:/subdir ===> parsedDir:  { 
  root: 'E:\\', 
  dir: 'E:\\', 
  base: 'subdir', 
  ext: '', name: 'subdir' }
tex0l commented 10 months ago

I think I had not understood correctly the documentation for path.parse:

β”Œβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”¬β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”
β”‚          dir        β”‚    base    β”‚
β”œβ”€β”€β”€β”€β”€β”€β”¬              β”œβ”€β”€β”€β”€β”€β”€β”¬β”€β”€β”€β”€β”€β”€
β”‚ root β”‚              β”‚ name β”‚ ext β”‚
" C:\      path\dir   \ file  .txt "
β””β”€β”€β”€β”€β”€β”€β”΄β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”΄β”€β”€β”€β”€β”€β”€β”΄β”€β”€β”€β”€β”€β”˜
(All spaces in the "" line should be ignored. They are purely for formatting.)

If I'm right, if the two following conditions are true, it means the tested directory is the root:

Therefore, if one of the two is false, I execute the mkdir.

Let me retry.

tex0l commented 10 months ago

Can you retry with v4.0.3-1 please @Type8.

Type8 commented 10 months ago

It works for me :)

Thanks...

tex0l commented 10 months ago

Released in 4.0.3, thanks!