-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
Config Tests - Check Filename #44962
Config Tests - Check Filename #44962
Conversation
@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? |
@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, 🌞 |
@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? |
@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. |
@captainrdubb I do not see how node.js would return |
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.
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. |
accidentally closed this. |
@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? |
@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. |
@captainrdubb make sure you try with node.js |
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, 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. |
@captainrdubb nice, so since you cannot reproduce in latest node.js I assume they fixed it meanwhile? |
@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? |
@captainrdubb great, can you file this as a bug to https://github.com/nodejs/node, would be interesting what they have to say. |
@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.
|
@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? |
@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. |
@captainrdubb your change is a bit crazy, I would suggest the following:
|
@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? |
@captainrdubb no worries, I can look into that change. |
fixes #44957 ConfigWatcher.onFileChange event incorrectly returns early when comparing filename of actual changed file to expected config filename