use separate process to probe if drive is safe to watch by vladima · Pull Request #14098 · microsoft/TypeScript

vladima · 2017-02-15T23:23:53Z

billti

Thanks! Still looking, but getting initial notes out now.

billti · 2017-02-16T04:45:18Z

+}
+const directoryName = process.argv[2];
+const fs: { watch(directoryName: string, options: any, callback: () => {}): any } = require("fs");
+fs.watch(directoryName, { recursive: true }, () => ({})).close();


I could think of a number of scenarios where this might be more defensive (e.g. check it exists, check it is a dir, check fs.watch returned a watcher object, etc...), but in all those cases seems the resulting error would result in correct behavior anyway in the listening process. However, might be worth a comment to note that this is deliberately "loose" here.

After thinking a bit more we should be more restrictive here. Will update

billti · 2017-02-16T04:46:23Z

+
    const sys = <ServerHost>ts.sys;
+    // use watchGuard process on Windows when node version is 4 or later
+    const useWatchGuard = process.platform === "win32" && parseInt(process.version.charAt(1)) >= 4;


In about 18 months when Node.js releases v10, this charAt check is going to break.

fair enough, I'll update similar code in sys as well

billti · 2017-02-16T05:12:27Z

+            // UNC path: extract server name
+            // //server/location
+            //         ^ <- from 0 to this position
+            const firstSlash = path.indexOf(directorySeparator, 2);


Any reason this might ever get a UNC path without the next separator (e.g. just "//someshare")? For safety, might want to check for indexOf returning -1 and return undefined (else below returns an empty string).

technically it might, will fix

billti · 2017-02-16T05:27:00Z

+    // use watchGuard process on Windows when node version is 4 or later
+    const useWatchGuard = process.platform === "win32" && parseInt(process.version.charAt(1)) >= 4;
+    if (useWatchGuard) {
+        const currentDrive = extractWatchDirectoryCacheKey(sys.resolvePath(sys.getCurrentDirectory()), /*currentDriveKey*/ undefined);


If I'm following this logic correctly, then local files on a system that doesn't use drive letters (e.g. Mac/Linux with "/user/bill/documents" etc...) will never get a cache key, and thus will never use the cache, always taking the hit to spawn the guard process. Is that right?

ah.... but of course. "useWatchGuard" in only true it process.platform == "win32". Don't mind me :-)

issue is specific to Windows version of libuv and does not exist on other platforms. that is why we use watch guard process only on Windows process.platform === "win32"

billti · 2017-02-16T05:41:02Z

+                    if (logger.hasLevel(LogLevel.verbose)) {
+                        logger.info(`Starting ${process.execPath} with args ${JSON.stringify(args)}`);
+                    }
+                    childProcess.execFileSync(process.execPath, args, { stdio: "ignore", env: { "ELECTRON_RUN_AS_NODE": "1" } });


Seems like it might be worth using the 'timeout' option on execFileSync. Seeing as 'fs.watch' by default will keep the process alive until 'close' has been called on the filewatcher, if the callback that calls 'close' in 'watchGuard' doesn't run for some reason, this would effectively hang tsserver. Seems like 1000ms should be a generous worst-case choice.

close is not called inside the callback but rather on return value of fs.watch

Actually, looking at watchGuard.js again, seem like it will always exit. The only risk being if fs.watch itself blocks for some reason, but then that's the same call that's going to run in tsserver anyway, so probably no value in the timeout.

billti · 2017-02-16T05:42:27Z

+
 var serverFile = path.join(builtLocalDirectory, "tsserver.js");
-compileFile(serverFile, serverSources, [builtLocalDirectory, copyright, cancellationTokenFile, typingsInstallerFile].concat(serverSources), /*prefixes*/ [copyright], /*useBuiltCompiler*/ true, { types: ["node"], preserveConstEnums: true });
+compileFile(serverFile, serverSources, [builtLocalDirectory, copyright, cancellationTokenFile, typingsInstallerFile, watchGuardFile].concat(serverSources), /*prefixes*/ [copyright], /*useBuiltCompiler*/ true, { types: ["node"] });


Was the removal of preserveConstEnums: true deliberate? Is it needed in this change?

no, bad merge

billti · 2017-02-16T05:52:11Z

+}
+const directoryName = process.argv[2];
+const fs: { watch(directoryName: string, options: any, callback: () => {}): any } = require("fs");
+fs.watch(directoryName, { recursive: true }, () => ({})).close();


Does the libuv bug definitely cause the process to crash synchronously on calling "fs.watch"? (i.e. it's not going to get to process.exit(0) before it the crash would occur).

it is crashes synchronously when creating watcher

billti · 2017-02-16T06:03:56Z

-            function isNode4OrLater(): boolean {
-                return parseInt(process.version.charAt(1)) >= 4;
-            }
+            const nodeVersion = getNodeMajorVersion();


Cool. parseInt(process.versions.node) would also have done the trick ;-)

good to know :) can change it to your proposal if you insist

billti

Thanks!

mhegazy · 2017-02-16T17:48:34Z

+            // //server/location
+            //         ^ <- from 0 to this position
+            const firstSlash = path.indexOf(directorySeparator, 2);
+            return firstSlash !== -1 ? path.substring(0, firstSlash).toLowerCase() : undefined;


path instead of undefined

mhegazy · 2017-02-16T17:50:02Z

+        return undefined;
+    }
+
+    export function isUNCPath(s: string): boolean {


no need for export

mhegazy · 2017-02-16T17:56:34Z

+            let status = cacheKey && statusCache.get(cacheKey);
+            if (status === undefined) {
+                if (logger.hasLevel(LogLevel.verbose)) {
+                    logger.info(`${cacheKey} not found in cache...`);


we should log the path as well.

mhegazy · 2017-02-16T17:56:58Z

+                    logger.info(`${cacheKey} not found in cache...`);
+                }
+                try {
+                    const args = [combinePaths(__dirname, "watchGuard.js"), path];


make sure LKG has the file name.

use dedicated process to determine if it is safe to watch folders

) use dedicated process to determine if it is safe to watch folders

…#14098) (#14124) * use separate process to probe if drive is safe to watch (#14098) use dedicated process to determine if it is safe to watch folders * added release-2.2

vladima added 3 commits February 14, 2017 23:27

use dedicated process to determine if it is safe to watch folders

60db2e4

run watchGuard once instead of having long-running process

2a3f8e2

Merge remote-tracking branch 'origin/master' into vladima/guardedWatch

b5693bf

vladima requested review from billti and mhegazy February 15, 2017 23:23

msftclas added the cla-not-required label Feb 15, 2017

billti suggested changes Feb 16, 2017

View reviewed changes

billti reviewed Feb 16, 2017

View reviewed changes

addressed PR feedback

e9b16c1

billti reviewed Feb 16, 2017

View reviewed changes

billti approved these changes Feb 16, 2017

View reviewed changes

mhegazy approved these changes Feb 16, 2017

View reviewed changes

vladima added 2 commits February 16, 2017 10:15

addressed PR feedback

d66ec20

remove unnecessary export

fa9d5b0

vladima merged commit 8c54bba into master Feb 16, 2017

vladima deleted the vladima/guardedWatch branch February 16, 2017 18:32

vladima added a commit that referenced this pull request Feb 16, 2017

use separate process to probe if drive is safe to watch (#14098)

bc95a97

use dedicated process to determine if it is safe to watch folders

vladima mentioned this pull request Feb 16, 2017

[release-2.1] use separate process to probe if drive is safe to watch (#14098) #14123

Merged

vladima added a commit that referenced this pull request Feb 16, 2017

use separate process to probe if drive is safe to watch (#14098)

ed99cdc

use dedicated process to determine if it is safe to watch folders

vladima mentioned this pull request Feb 16, 2017

[release-2.2] use separate process to probe if drive is safe to watch (#14098) #14124

Merged

vladima added a commit that referenced this pull request Feb 16, 2017

use separate process to probe if drive is safe to watch (#14098) (#14123

b9ac264

) use dedicated process to determine if it is safe to watch folders

weswigham mentioned this pull request Jul 31, 2017

Update LKG #17538

Merged

microsoft locked and limited conversation to collaborators Jun 19, 2018

Conversation

vladima commented Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

billti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billti Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladima Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billti Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vladima commented Feb 15, 2017 •

edited

Loading

billti Feb 16, 2017 •

edited

Loading

vladima Feb 16, 2017 •

edited

Loading

billti Feb 16, 2017 •

edited

Loading