Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs.rmdir on Windows throws ENOENT instead of ENOTDIR #18014

Closed
saschanaz opened this issue Jan 6, 2018 · 7 comments
Closed

fs.rmdir on Windows throws ENOENT instead of ENOTDIR #18014

saschanaz opened this issue Jan 6, 2018 · 7 comments
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system.

Comments

@saschanaz
Copy link

  • Version: v9.3.0
  • Platform: Windows 10
  • Subsystem:
> fs.statSync("emmake")
Stats {
  dev: 3066473769,
  mode: 33206,
  nlink: 1,
  uid: 0,
  gid: 0,
  rdev: 0,
  blksize: undefined,
  ino: 105271641289789120,
  size: 548,
  blocks: undefined,
  atimeMs: 1515062172219.6804,
  mtimeMs: 1515062172220.681,
  ctimeMs: 1515062172220.681,
  birthtimeMs: 1496261901857.4617,
  atime: 2018-01-04T10:36:12.220Z,
  mtime: 2018-01-04T10:36:12.221Z,
  ctime: 2018-01-04T10:36:12.221Z,
  birthtime: 2017-05-31T20:18:21.857Z }
> fs.rmdirSync("emmake")
Error: ENOENT: no such file or directory, rmdir 'C:\Users\sasch\Documents\GitHub\emsdk\emscripten\incoming\emmake'
    at Object.fs.rmdirSync (fs.js:866:18)

It should throw ENOTDIR instead of ENOENT as there is an entry.

@vsemozhetbyt vsemozhetbyt added errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. labels Jan 6, 2018
@bnoordhuis
Copy link
Member

That error is reported by the operating system. There is not enough information for node.js/libuv to distinguish between 'path does not exist' and 'path is not a directory' without extra manual checks - but as that's prone to race conditions, libuv doesn't do that.

To summarize: it's probably a wontfix.

@tniessen
Copy link
Member

tniessen commented Jan 7, 2018

That error is reported by the operating system. There is not enough information for node.js/libuv to distinguish between 'path does not exist' and 'path is not a directory' without extra manual checks - but as that's prone to race conditions, libuv doesn't do that.

@bnoordhuis Using RemoveDirectory instead of _wrmdir might solve this problem upstream. RemoveDirectory will cause ERROR_DIRECTORY if the path does not point to a directory, and ERROR_FILE_NOT_FOUND if the path does not exist. I might have time to open a PR upstream, unless there was a reason not to use RemoveDirectory in the first place? I am not sure how difficult the error code translation is...

By the way, according to the documentation of _wrmdir, the function should return ENOTEMPTY instead of ENOENT. This might be an implementation bug in the OS / CRT.

@bnoordhuis
Copy link
Member

unless there was a reason not to use RemoveDirectory in the first place?

I can't find the thread again but the issue was that RemoveDirectory() didn't return ERROR_DIRECTORY but something like ERROR_INVALID_NAME. That said, maybe you and @saschanaz can check if this patch works as expected?

diff --git a/deps/uv/src/win/error.c b/deps/uv/src/win/error.c
index 9b03bfef6b..1ec3d6e27f 100644
--- a/deps/uv/src/win/error.c
+++ b/deps/uv/src/win/error.c
@@ -131,7 +131,7 @@ int uv_translate_sys_error(int sys_errno) {
     case WSAENETUNREACH:                    return UV_ENETUNREACH;
     case WSAENOBUFS:                        return UV_ENOBUFS;
     case ERROR_BAD_PATHNAME:                return UV_ENOENT;
-    case ERROR_DIRECTORY:                   return UV_ENOENT;
+    case ERROR_DIRECTORY:                   return UV_ENOTDIR;
     case ERROR_FILE_NOT_FOUND:              return UV_ENOENT;
     case ERROR_INVALID_NAME:                return UV_ENOENT;
     case ERROR_INVALID_DRIVE:               return UV_ENOENT;
diff --git a/deps/uv/src/win/fs.c b/deps/uv/src/win/fs.c
index 11c7c13edd..e4e4c58d7a 100644
--- a/deps/uv/src/win/fs.c
+++ b/deps/uv/src/win/fs.c
@@ -723,8 +723,11 @@ void fs__write(uv_fs_t* req) {
 
 
 void fs__rmdir(uv_fs_t* req) {
-  int result = _wrmdir(req->file.pathw);
-  SET_REQ_RESULT(req, result);
+  if (RemoveDirectoryW(req->file.pathw)) {
+    SET_REQ_SUCCESS(req);
+  } else {
+    SET_REQ_WIN32_ERROR(req, GetLastError());
+  }
 }
 
 

@tniessen
Copy link
Member

@bnoordhuis That's exactly what I had in mind and it seems to work as expected:

patch

I am not sure whether this will have implications for other APIs, depending on whether Windows uses ERROR_DIRECTORY for other error conditions as well.

bnoordhuis added a commit to bnoordhuis/libuv that referenced this issue Jan 12, 2018
Use RemoveDirectoryW() and remap ERROR_DIRECTORY from UV_ENOENT
to UV_ENOTDIR so that attempted removal of a non-directory produces
the right (and legible) error message.

Fixes: nodejs/node#18014
@bnoordhuis
Copy link
Member

It might have been an XP-only issue or a buggy driver or something like that. Filed libuv/libuv#1698.

bnoordhuis added a commit to bnoordhuis/libuv that referenced this issue Jan 19, 2018
Reverted for breaking `test/parallel/test-child-process-cwd.js` from the
Node.js test suite.  Instead of ENOENT when trying to remove a directory
that does not exist, it started failing with ENOTDIR.

This reverts commit 15f29dc.

PR-URL: libuv#1717
Refs: nodejs/node#18014
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
@bnoordhuis
Copy link
Member

bnoordhuis commented Jan 19, 2018

I can't find the thread again but the issue was that RemoveDirectory() didn't return ERROR_DIRECTORY but something like ERROR_INVALID_NAME.

See libuv/libuv@9f07a36, it was that it also returns ERROR_DIRECTORY when the directory is missing. Reverted upstream.

I'm leaving this closed because I'm not going to revisit it. For anyone wanting to work on this, libuv is the place where the action happens.

@saschanaz
Copy link
Author

MSDN documents ERROR_DIRECTORY as The directory name is invalid, which doesn't say how invalid it is. Sad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

No branches or pull requests

4 participants