win,fs: fix inconsistencies between platforms for rename #1941

Shivang44 · 2018-08-08T21:15:42Z

Renaming directories has odd inconsistencies between Unix and Win
platforms. For example, renaming dirA to dirB when dirB is empty fails
on Win while succeeds on Linux. This commit fixes that by removing dirB
if dirB is empty. Additionally, renaming dirA to dirB when dirB is NOT
empty, on Linux, returns a ENOTEMPTY error, while on Win, returns a
EPERM error. This fixes this consistency be returning a ENOTEMPTY error
on windows as well.

Related to nodejs/node#21957

Note to reviewers: I am very new to C/C++ and I am sure I made mistakes,
both in my method and my test case. I would appreciate any in-depth feedback
on my code. Thank you!

Note about tests: The tests pass, but on some rare occasions they will fail. I assuming there are just flaky tests?

Renaming directories has odd inconsistencies between Unix and Win platforms. For example, renaming dirA to dirB when dirB is empty fails on Win while succeeds on Linux. This commit fixes that by removing dirB if dirB is empty. Additionally, renaming dirA to dirB when dirB is NOT empty, on Linux, returns a ENOTEMPTY error, while on Win, returns a EPERM error. This fixes this consistency be returning a ENOTEMPTY error on windows as well. Related to nodejs/node#21957 Note to reviewers: I am very new to C/C++ and I am sure I made mistakes, both in my method and my test case. I would appreciate any in-depth feedback on my code. Thank you!

Shivang44 · 2018-08-08T21:21:42Z

Also, I am not sure if that pragma is in the best spot (or if I should even be using it). I added it because it will not compile without that directive.

Shivang44 · 2018-08-16T15:32:05Z

I realize windows is not that popular of platform for nodejs, but I feel fixing these inconsistencies is important. Can somebody please review this PR?

cjihrig

Note about tests: The tests pass, but on some rare occasions they will fail. I assuming there are just flaky tests?

It depends which test(s) you're talking about. Can you be more specific about individual tests and platforms?

Overall, I'm a little torn on this. If it aligns the behavior across platforms, that's good. However, it also introduces a bit of a potential race condition due to the extra file system calls. There is also the question of whether this would classify as a bug fix or breaking change. I'd like to know what other collaborators think.

cjihrig · 2018-08-17T02:41:05Z

src/win/fs.c

@@ -37,6 +37,9 @@

 #include <wincrypt.h>

+#pragma comment(lib, "Shlwapi.lib")


We don't really use this style in the codebase. There is currently #1890, which adds it, but it's unclear that it will get merged.

cjihrig · 2018-08-17T02:41:37Z

src/win/fs.c

+  WCHAR* pathw = req->file.pathw;
+  WCHAR* new_pathw = req->fs.info.new_pathw;
+
+   // These checks allow us to mimic the rename behavior on Unix platforms when


The indentation on these comments is off. Also, please use /* */ style comments.

cjihrig · 2018-08-17T02:42:16Z

src/win/fs.c

@@ -1301,6 +1304,34 @@ static void fs__fstat(uv_fs_t* req) {


 static void fs__rename(uv_fs_t* req) {
+  WCHAR* pathw = req->file.pathw;


Please separate the declaration and assignment. So:

WCHAR* pathw; pathw = req->file.pathw;

cjihrig · 2018-08-17T02:43:16Z

src/win/fs.c

+   // like on Unix, rather than a EPERM error that MoveFileXW will throw on
+   // a nonempty directory.
+  if (PathIsDirectoryW(new_pathw)) {
+


Please remove the blank lines around the ifs and else here and below.

cjihrig · 2018-08-17T02:44:25Z

src/win/fs.c

+    if (PathIsDirectoryEmptyW(new_pathw)) {
+
+      // Attempt deletion of empty directory so MoveFileExW will succeed on rename.
+      if(!RemoveDirectoryW(new_pathw)) {


Space after if.

cjihrig · 2018-08-17T02:45:59Z

src/win/fs.c

+
+      // Attempt deletion of empty directory so MoveFileExW will succeed on rename.
+      if(!RemoveDirectoryW(new_pathw)) {
+            SET_REQ_WIN32_ERROR(req, GetLastError());


Indentation is off.

cjihrig · 2018-08-17T02:55:56Z

src/win/fs.c

+   // If the new path is a directory, check if it is empty.
+   // If it is, remove the directory so MoveFileExW and rename will succeed.
+   // If the directory is not empty, return by throwing a ENOTEMPY error,
+   // like on Unix, rather than a EPERM error that MoveFileXW will throw on


MoveFileXW -> MoveFileExW ?

cjihrig · 2018-08-17T02:57:55Z

src/win/fs.c

+
+    } else {
+      // Directory is not empty, so throw a non-empty directory error.
+      SET_REQ_WIN32_ERROR(req, ERROR_DIR_NOT_EMPTY);


I think you could just use SET_REQ_UV_ERROR() here.

cjihrig · 2018-08-17T02:59:54Z

test/test-fs.c

@@ -2805,6 +2805,38 @@ TEST_IMPL(fs_rename_to_existing_file) {
  return 0;
 }

+TEST_IMPL(fs_rename_to_existing_empty_directory) {


Should there also be a test case for ENOTEMPY?

cjihrig · 2018-08-17T03:01:43Z

src/win/fs.c

@@ -1301,6 +1304,34 @@ static void fs__fstat(uv_fs_t* req) {


 static void fs__rename(uv_fs_t* req) {
+  WCHAR* pathw = req->file.pathw;


This variable appears to be unused.

Shivang44 · 2018-08-21T01:34:15Z

@cjihrig Thanks for reviewing the PR! I will revise the PR according to your comments, but want to discuss it before I do so time is not wasted if this change is deemed to be not right by other maintainers.

Overall, I'm a little torn on this. If it aligns the behavior across platforms, that's good. However, it also introduces a bit of a potential race condition due to the extra file system calls. There is also the question of whether this would classify as a bug fix or breaking change. I'd like to know what other collaborators think.

Which part of the code creates a race condition? It was my understanding that the calls like PathIsDirectory RemoveDirectory block until they are returned? So there wouldn't be a race condition right? Again, my understanding of these calls and C are limited.

It depends which test(s) you're talking about. Can you be more specific about individual tests and platforms?

I will rerun the tests when I get a chance and let you know!

Overall, I made this PR because I thought this inconsistency in fs was an important one to fix. If other maintainers think that this difference across platforms is not that big of a deal, then I am completely okay with skipping this PR all together!

cjihrig · 2018-08-21T14:23:39Z

Which part of the code creates a race condition? It was my understanding that the calls like PathIsDirectory RemoveDirectory block until they are returned?

I was referring to the window of time between the calls. It's possible for another process to modify the contents of the directory while this function is executing. For example, the directory could go from empty to not empty between checking if it's empty and trying to remove it. It's definitely an edge case, but that's why I wanted to see what others thought.

refack · 2018-08-21T19:37:15Z

Hello @Shivang44,

I feel fixing these inconsistencies is important.

I'm sure you are aware there are numerous other inconsistencies, for example; case sensitive names, timestamp accuracy, no links on FAT, different ACL mechanisms.

For example, renaming dirA to dirB when dirB is empty fails on Win while succeeds on Linux. This commit fixes that by removing dirB if dirB is empty.

IMHO implementing the complex solution you suggest should be left for downstream projects (e.g. node like mkdirp, or a userland module)

As for returning ENOTEMPY I think that is a breaking change. Since you are already making breaking changes, I suggest returning ENOTEMPY for both cases (if dirB is empty or not). I would go as far as to suggest to return ENOTEMPY even on POSIX, but this requires buy-in from the Collaborators.

refack · 2018-08-21T19:49:42Z

P.S. I just found what I believe is a bug on Windows:

> mkdir dirA
> echo " " > dirB
> node -e "fs.renameSync('dirA', 'dirB')"

>

This will overwrite dirB.

For reference, on Ubuntu 16.04:

$ mkdir dirA
$ echo " " > dirB
$ node -e "fs.renameSync('dirA', 'dirB')"
fs.js:119
    throw err;
    ^

Error: ENOTDIR: not a directory, rename 'dirA' -> 'dirB'

Shivang44 · 2018-09-01T00:50:44Z

Due to the lack of interest as well as the above mentioned (potential) issues, I will be closing this PR. If other maintainers think that this change would be a good addition to libuv, I would be happy to reopen it work on it!

Thanks for your comments and help @cjihrig @refack

Shivang44 mentioned this pull request Aug 8, 2018

fs.rename doesn't work as documented nodejs/node#21957

Closed

cjihrig reviewed Aug 17, 2018

View reviewed changes

refack added windows feature-request enhancement and removed feature-request labels Aug 21, 2018

Shivang44 closed this Sep 1, 2018

cjihrig mentioned this pull request Nov 20, 2018

doc: Change doc on fs.rename api w.r.t to windows behaviour nodejs/node#22014

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

win,fs: fix inconsistencies between platforms for rename #1941

win,fs: fix inconsistencies between platforms for rename #1941

Shivang44 commented Aug 8, 2018

Shivang44 commented Aug 8, 2018

Shivang44 commented Aug 16, 2018

cjihrig left a comment

cjihrig Aug 17, 2018

cjihrig Aug 17, 2018

cjihrig Aug 17, 2018

cjihrig Aug 17, 2018

cjihrig Aug 17, 2018

cjihrig Aug 17, 2018

cjihrig Aug 17, 2018

cjihrig Aug 17, 2018

cjihrig Aug 17, 2018

cjihrig Aug 17, 2018

Shivang44 commented Aug 21, 2018 •

edited

Loading

cjihrig commented Aug 21, 2018

refack commented Aug 21, 2018

refack commented Aug 21, 2018 •

edited

Loading

Shivang44 commented Sep 1, 2018 •

edited

Loading

		@@ -37,6 +37,9 @@

		#include <wincrypt.h>

		#pragma comment(lib, "Shlwapi.lib")

		@@ -1301,6 +1304,34 @@ static void fs__fstat(uv_fs_t* req) {


		static void fs__rename(uv_fs_t* req) {
		WCHAR* pathw = req->file.pathw;

win,fs: fix inconsistencies between platforms for rename #1941

win,fs: fix inconsistencies between platforms for rename #1941

Conversation

Shivang44 commented Aug 8, 2018

Shivang44 commented Aug 8, 2018

Shivang44 commented Aug 16, 2018

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shivang44 commented Aug 21, 2018 • edited Loading

cjihrig commented Aug 21, 2018

refack commented Aug 21, 2018

refack commented Aug 21, 2018 • edited Loading

Shivang44 commented Sep 1, 2018 • edited Loading

Shivang44 commented Aug 21, 2018 •

edited

Loading

refack commented Aug 21, 2018 •

edited

Loading

Shivang44 commented Sep 1, 2018 •

edited

Loading