Config Tests - Check Filename

[captainrdubb]

fixes #44957 ConfigWatcher.onFileChange event incorrectly returns early when comparing filename of actual changed file to expected config filename

[bpasero]

@captainrdubb just fyi the tests are not failing for me and also not for the CI builds, so I am not sure why you see test failures. Which OS are you on?

[captainrdubb]

@bpasero Windows 10 Pro N 10.0.16299 version 1709

Not sure why machine is special 😞. At least this may provide an additional safeguard against the behavior of an external dependency, 🌞

[bpasero]

@captainrdubb I do not understand this fix, sorry. The intent of that check is to ignore any file events that happen to files other than the config file to watch for. node.js will send the filename of the file that changes as argument and we do a simple triple-equals check on that name.

Are you saying you are getting different filenames even though the change is in the config file?

[captainrdubb]

@bpasero No problem. Take a look at this issue #44957. There is an assumption that node is returning the last fragment of the changed file's uri. Unfortunately, we can't assume that. It's possible that node does not always properly parse the uri and returns more than it should. For example, it returns "776728/config.json" instead of "config.json". In this case, a triple equals fails. As a solution we can ensure that the final part of the file name is correct, even if node doesn't parse out the file name perfectly. I think that it's safe to assume that the correct path is monitored, so there is no need to worry about the entire file name returned, as long as the last part exactly matches what we expect it to be. There probably is a simpler way, but my intention is to ensure that even files with similar names would not pass the check, but file names that are correct will pass, despite the parsing problem.

[bpasero]

@captainrdubb I do not see how node.js would return 776728/config.json, what folder is 776728 (compare with their docs: https://nodejs.org/docs/v7.9.0/api/fs.html#fs_fs_watch_filename_options_listener there is no mention of the event returning anything but the filename)? And even if there are more segments returned in the event, we always want the top level config.json and never any children because we are only interested in config.json?

[captainrdubb]

@bpasero

I do not see how node.js would return 776728/config.json, what folder is 776728

So the watching test calls the testFile function, where a new directory is made beneath the temp directory. In my file system it looks like ""C:\Users<part of my username>~1\AppData\Local\Temp\vsctests\900c9432-394f-4996-92d6-053f018728ee\config\900c9432-394f-4996-92d6-053f018728ee". Then, the name of the changed file returned by node is "f018728ee\config.json". "f018728ee" is the last part of the UUID.

(compare with their docs: . . . there is no mention of the event returning anything but the filename)?

Yeah, I looked at the node docs as well, hoping to see some indicator. There is the following section.

I must confess, fs.watch does not give a warm fuzzy feeling haha. Maybe the issue lies within ReadDirectoryChangesW.

A slightly unrelated issue, if the watching test fails, then cleanup never happens and my temp directory gets junky.

[captainrdubb]

accidentally closed this.

[bpasero]

@captainrdubb that sounds more like a bug in node.js to me (especially since only a portion of the actual path is returned), can you reproduce if you create a little node.js script where you watch on such a path and then change a file within to see what you get back in the callback?

[captainrdubb]

@bpasero I created a node script, it returned a normal filename.

Debugging the config tests in Chrome leads me to the function below in fs.js.

I know this._handle is FSEvent from fs_event_wrap.cc and that the funky file name is emitted from there. Unfortunately, I'm using Windows, so I'm having a hard time figuring out how to get the C++ stack trace or even a core dump.

[bpasero]

@captainrdubb make sure you try with node.js 7.9.0, that is the one we ship in VS Code.

[captainrdubb]

@bpasero

Eureka!!

caveat: i did not look into Node c++ land, but I was able to reproduce it using version 8.9.4

The function os.tmpDir returns a path with the 8.3 short name for the user name, example "C:\Users\<FIRST SIX CHARS OF NAME>~1\AppData\Local\Temp".

The Windows api returns the changed file name including the long version of the user name, example,
"C:\Users\<actual 18 char user name>\AppData\Local\Temp".

Pretend we are monitoring, "C:\Users\<8.3 file name>\AppData\Local\Temp\Configuration", expecting the file "config.json" to change. We would then expect the FSWatcher change event to return "config.json" as the name of the changed file. Instead, it returns something like "figuration\config.json", depending on the difference in length between the 8.3 file name and the long file name.

It seems that node is saving an index so that it can quickly determine the part of the uri that is the file name, but the uri passed to fs.watch is a different length than the uri returned by the Windows api.

[bpasero]

@captainrdubb nice, so since you cannot reproduce in latest node.js I assume they fixed it meanwhile?

[captainrdubb]

@bpasero oh no, I did reproduce it. I'll edit my last comment. I'm using 8.9.4

Does node have a way to return the actual user name in the temp folder uri?
If not, does node have a way to get the actual user name?
If not, is the regex solution the simplest?

[bpasero]

@captainrdubb great, can you file this as a bug to https://github.com/nodejs/node, would be interesting what they have to say.

[captainrdubb]

@bpasero Will do.

I discovered that it's not a search term, but a 8.3 file name replacement created by the Windows file system for a name that is too long.

Both FAT and NTFS use the Unicode character set for their names, which contain several forbidden characters that MS-DOS cannot read in any file name. To generate a short MS-DOS-readable file name for a file, Windows 2000 deletes all of these characters from the long file name and removes any spaces. Because an MS-DOS-readable file name can have only one period, Windows 2000 also removes all extra periods from the file name. Next, Windows 2000 truncates the file name, if necessary, to six characters and appends a tilde ( ~ ) and a number. For example, each nonduplicate file name is appended with ~1 . Duplicate file names end with ~2 , then ~3 , and so on. After the file names are truncated, the file name extensions are truncated to three or fewer characters. Finally, when displaying file names at the command line, Windows 2000 translates all characters in the file name and extension to uppercase. full article

[bpasero]

@captainrdubb thanks, given this behaviour I do not see how to best fix this because we do not know which file was changed, unless we somehow get the absolute path, would you agree?

[captainrdubb]

@bpasero To me there really isn't a gray area. We are already taking for granted that we are monitoring the right directory. As it stands, we already assume that the directory is correct, only checking the file name itself. Now, we discover that the file name returned isn't perfect, including part of the parent directory in the name. In this case, isn't it safe to discard the parent directory part of the returned file name, since we're assuming it's in the right directory in the first place? If discarding the cruft is acceptable, then we're back where we started, just checking the file name itself. We do not need to deem the FSWatcher change event completely unreliable. In my mind we're not getting the "wrong" file name. We're just getting a little more than we asked for, so we can say, "no thanks" and keep the part that we want.

Issue submitted to nodejs nodejs/node/issues/19170.

[bpasero]

@captainrdubb your change is a bit crazy, I would suggest the following:

• use basename from node.js to get the true basename of the file instead of running a complicated RegEx
• add a isWindows check to only do that if we are running on Windows
• add a comment pointing to your node.js issue and add a TODO@Ben so that I can track this in the future

[captainrdubb]

@bpasero Honestly, this seems crazy as well. I'm not able to find a reliable, dynamic way to create a folder in the temp directory and it seems a little much. Provided there isn't a "real" file syncing problem, beyond what the tests have already proved, am I correct in believing that you'd rather leave the code alone?

[bpasero]

@captainrdubb no worries, I can look into that change.