storage.js: check fsync capability from return code rather than using process.platform heuristics

[bitmeal]

This pull request was originally filed against JamesMGreene/nestdb

Why: Currently fsync on the directory containing the database is skipped if process.platform equals win32 or win64. This check is considered insufficient and should be replaced by checks evaluating the error responses of operations, as there exists more than the default Linux (and possibly assumed posix compatible) and Windows filesystem backend/driver/adapter implementations.

An Example for (multiple) implementations of filesystem backends is BrowserFS, where most filesystems are not backed by physical storage and thus do neither use the OSs' implementation, nor libuvs' wrappers (as it may be ran in some browser). In this example, using nestdbs' node variant in the browser, with default webpack polyfills and BrowserFS as a node fs compatible filesystem adapter implementation, is completely possible, but chokes because of the is windows-check for determining fsync directory capability.

Testing for the actual error responses of filesystem operations allows to silently ignore the incapability, based on the actual implementations' capabilities.

What is being changed: No API changes. No observable behavior changes with default node fs module.

Changing the original behavior of branching out of flushToStorage()

// original
if (flags === 'r' && (process.platform === 'win32' || process.platform === 'win64')) { return callback(null); }
  fs.open(filename, flags, function (err, fd) {
    // ...
  });

... to checks of the fs operation return codes.

When calling open() on a directory yields an EISDIR error, we can safely assume no fsync dir support. Access permissions and rights for the current user should be checked on open() and yield an EPERM error if not sufficient. An EPERM error should thus never be returned for a call to fsync() on fsync dir capable implementations, as permissions have already been checked. It is thus assumed, that a EPERM (returned on Windows) or EISDIR error safely indicates a fsync dir incapability and not a permission error.

// new
  fs.open(filename, flags, function (err, fd) {
    if (err) {
      return callback((err.code === 'EISDIR' && options.isDir) ? null : err);
    }
    fs.fsync(fd, function (errFS) {
      fs.close(fd, function (errC) {
        if ((errFS || errC) && !((errFS.code === 'EPERM' || errFS.code === 'EISDIR') && options.isDir)) {
          // ...
        } else {
          return callback(null);
        }
      });
    });
  });

• All tests passing on Windows 10, Alpine v3.11 and Debian Buster.
• No observable differences in behavior (types and locations of thrown errors) to original implementation found, when dropping write permission for database file or directory (regarding the check for EPERM as indicator) (platforms as above)
• make byline test run with windows line endings to validate storage behavior on windows

• add shebang in EMFILE exhaustion test script

  • [ ] Add or update tests, if applicable
  • [x] Update code comments, if applicable
  • [ ] Update documentation, if applicable

[arantes555]

@bitmeal Thanks for the PR ! I'm not the maintainer of this project, @tex0l is, so he has the final word about this, but it looks good to me ! Just a small nitpick that I know he's gonna want fixed : your PR changes a const back to a var , and a few arrow-fucntions to normal functions, as the code of this NeDB fork was updated to a more modern coding style. If you could update your PR, that would be great

[tex0l]

I agree on the principle of this PR; however, there is a major overhaul ongoing (#11) in which the changes need to be ported.

Should we make a v2 patch out of this or include it in the v3 major?

[tex0l]

Ok, let's make a patch out of this, I agree with @arantes555 on the changes to be made. I'll make the comments inline.

[bitmeal]

I performed the requested changes. To be fair, I did not really look into your style guide and linting, as I just took over the code from me previous PR to nestdb - my bad!

The changes have been amended to the previous commit to not clutter your commit history.