win,fs: don't modify global file translation mode

[jblazquez]

Back in 25175c when the filesystem API was initially implemented on Windows, uv_fs_open was internally implemented by calling the _open function. Shortly thereafter, in e1af07, the implementation was changed to use CreateFile instead. However, the code in uv_fs_init that was modifying the global _fmode variable to set the default file translation mode to binary was left in place, and has been there for 8 years.

A library like libuv shouldn't be changing this global state, because the embedding application may need to use a different default file translation mode (this is how we found out libuv was doing this). Also, since the change to use CreateFile there's no longer a need for libuv to change this mode anyway, because it no longer uses either _open or _pipe, which are the functions that this global mode variable affects as per the docs.

This change stops libuv from changing this global state. All tests still pass after this change.

[bnoordhuis]

LGTM, thanks. Good catch too.

Perhaps you can remove uv_fs_init() altogether since it's a stub now? It's called from core.c:

$ git grep -n uv_fs_init src/win
src/win/core.c:208:  uv_fs_init();
src/win/fs.c:140:void uv_fs_init(void) {
src/win/internal.h:246:void uv_fs_init(void);

[bzoz]

LGTM, CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/1422/

[jblazquez]

Thanks, added a new commit to delete the now unused uv_fs_init.

[bnoordhuis]

@jblazquez This PR seems to make a test fail consistently on Windows (failing on all hosts):

06:49:12 not ok 34 - fs_async_sendfile
06:49:12 # exit code 3
06:49:12 # Output from process `fs_async_sendfile`:
06:49:12 # Assertion failed in test-fs.c on line 1122: 65542 == lseek(f, 65536, SEEK_CUR)

Can you take a look?

I was kind of afraid libuv users implicitly depending on the value of _fmode might turn out to be issue, and seeing that our own test suite does, that's probably a legitimate fear.

Here is a Node.js integration CI for good measure: https://ci.nodejs.org/view/libuv/job/libuv-in-node/98/

[cjihrig]

Surprisingly, the Node integration CI seems to be OK.

[jblazquez]

@bnoordhuis you're right, that fs_async_sendfile test was failing with my changes and I missed it, thanks. The reason was that it was assuming binary file mode but the default was text and the \n was being converted to \r\n on Windows:

static void sendfile_setup(int f) {
  ASSERT(6 == write(f, "begin\n", 6)); // <- was actually writing 7 bytes
  ASSERT(65542 == lseek(f, 65536, SEEK_CUR));
  ASSERT(4 == write(f, "end\n", 4));
}

I pushed a new change to make the test runner change the default file translation mode to binary, because that's what the tests are expecting.

I agree that it is possible some existing users of libuv may be implicitly depending on the file mode being set to binary indirectly by the library. The _fmode setting code had been there for 8 years after all. I guess we would need to decide whether we want to keep the _fmode setting code for the sake of those existing users, or go ahead with this PR for the sake of future users that may not want the mode changed under them. This is my thinking:

1. If we keep the _fmode setting code then existing users don't need to do anything, but future users who depend on the default file translation mode would need to reset the mode to text if that's what they need. This has to be done after the first call to any libuv library function, because that's what implicitly calls uv_fs_init and changes the mode to binary. For some users it will be trivial to add their own _set_fmode call after calling the first libuv function, but for others it might not be, for example if they don't directly use libuv but rather link to another library that internally does and they can't control when exactly that library will call the first libuv function and change this global mode. This is the main drawback of changing global process state from a library.

2. If we remove the _fmode setting code in libuv then future users don't need to do anything, but some existing users may need to set the default file translation mode themselves when they didn't have to before. They can do this at the start of their program or wherever is convenient for them. I can't think of any reasonable scenario where this would pose a significant challenge, other than of course the need to add some code to their application.

I understand if libuv decides that any chance of breaking an existing user is not acceptable and it would be better to keep this global state change in the library for the sake of those users, but it would be a shame being unable to ever walk back this code written so early in the project. Would a warning in the release notes or in the documentation about the file translation mode help appease these concerns?

[bnoordhuis]

@libuv/collaborators It would be good if you could weigh in, and @vtjnash, perhaps you can check whether this breaks Julia?

My own line of thought is that touching the global file mode in a library is a bug so I am (with some trepidation) in favor of landing this change.

We would need to call it out clearly in the release notes, though. A note at the top of docs/src/fs.rst probably won't hurt either.

[bzoz]

It sure feels like a bug, I'm +1 on lading this.

[vtjnash]

It won't break Julia since we don't use this LTS branch, and it's already gone from master. It does seem like a breaking change to do for other consumers though, especially since it breaks the tests.

[cjihrig]

I'm OK with landing it.

[ptlomholt]

Hi, just wanted to point out that there is a similar issue (e.g modifying process wide settings) with SetErrorMode() on Windows. I filed #1327 some time back and it was rejected out of fear for breaking users that depended on the existing behavior...

[saghul]

+1 to landing.

[bnoordhuis]

Landed in f9adfb2, thanks! Let's see how many bug reports we receive. :-)

[cjihrig]

I rebased this commit out of the v1.x branch for the 1.30.1 patch release. Now that the release is out, I've landed this again as c905e0b.