Possible Windows Node 6 regression with spawn and a substed drive #6500

xzyfer · 2016-05-01T10:21:44Z

Version:
v6.0.0
Platform:
Windows 32 and 64 bit
Subsystem:

I've noticed a minor breaking change in either child_process.spawn or path.resolve (currently unclear), on Windows with Node 6. This issue appears to crop up when a Node script is execute from a SUBST'd path.

The production steps are tricky so I have created a reproduction in a repo that runs its tests in AppVeyor.

The text was updated successfully, but these errors were encountered:

xzyfer · 2016-05-01T10:24:34Z

This may be related to #5950.

saper · 2016-05-01T10:25:13Z

Wow! I'm amazed.

saper · 2016-05-01T10:30:34Z

@xzyfer can you rename the issue to "substed" ? (or reference "drive created by subst.exe") - only few MS-DOS die-hards like me remember, what a "substed drive" is.

xzyfer · 2016-05-01T10:32:43Z

updated

Node 6.x has trouble probably including modules from the substed drivers. Since we use use subst only to set symbols paths in the PDB file, let's revert to the C: drive when doing tests. Workaround for: nodejs/node#6500

Node 6.x has trouble including modules from MS-DOS drives created with subst.exe. Since we use use subst only to set symbols paths in the PDB file, let's revert to the C: drive when doing tests. Workaround for: nodejs/node#6500

Node 6.x has trouble including modules from MS-DOS drives created with subst.exe. Since we use use subst only to set symbols paths in the PDB file, let's revert to the C: drive when doing tests. Workaround for: nodejs/node#6500 [ci skip]

Node 6.x has trouble including modules from MS-DOS drives created with subst.exe. Since we use use subst only to set symbols paths in the PDB file, let's revert to the C: drive when doing tests. Workaround for: nodejs/node#6500

saper · 2016-05-02T10:35:43Z

I have built node 7.0.0 (fa542eb) and the issue is still there. Reverting de1dc0a fixes the issue - npm test done one a substed drive passes.

xzyfer · 2016-05-02T14:44:37Z

Nice find @saper

jasnell · 2016-05-02T15:39:09Z

Yep, de1dc0a fixed another bug but led to some other regressions. There is work underway to find a workaround that would address both sets of concerns but it's non-trivial. This is queued up already for @nodejs/ctc discussion this week to see if we need to revert or find another path forward.

kzc · 2016-05-02T16:12:41Z

Is SUBST the Windows equivalent of a symlink?

mkdir c:\projects
cd c:\projects
git clone https://github.com/xzyfer/node-6-native-ext-bug.git
cd node-6-native-ext-bug
subst s: c:\projects
npm install
npm test

If you cd to s:\node-6-native-ext-bug does npm test run successfully?

saper · 2016-05-02T18:39:51Z

Subst is the ancient feature, coming from MSDOS times. It is more like Unix nullfs - mounting some subdirectory under the new name. But Unix does not have drive letters.

and yes, it failes when you are "on" a substed drive. Compiling from C:\projects works.

kzc · 2016-05-02T19:45:29Z

Thanks @saper. In the DOS days I'm sure I used SUBST but knowledge of it was pushed out of my brain.

So a SUBST'd drive is similar to a symlink. However unlike a symlink, it changes the original source directory into the logical link and treats the new drive as the realpath? How odd. This seems backwards.

If this same example were run with node 5.x or earlier, does npm test still run within the subst'd directory s:\node-6-native-ext-bug? If my theory is correct it won't.

saper · 2016-05-02T20:15:42Z

Yes, all the tests ran correctly.

realpath is nothing magical, it tries to do its job to some extent. Node uses libuv, and its implementation of realpath for Windows uses GetFinalPathNameByHandle Windows API, which is not enough in this case. One had to use QueryDosDevice API. But there is also JOIN.EXE to makes things even harder. (Or ASSIGN.EXE, which seems to be gone from the most recent Windows releases).

kzc · 2016-05-02T20:33:57Z

@saper Okay this should be fixable. To your point, libuv fs__realpath_handle should implement the algorithm in http://pdh11.blogspot.ca/2009/05/pathcanonicalize-versus-what-it-says-on.html for realpathing of SUBST drives which should fix the situation.

Can you please confirm that if you use windows symlinks instead of SUBST - does npm test work in both the original and symlinked directory with node 6.0.0? Not knowing anything about Windows symlinks, google tells me Admin privileges may be required.

saper · 2016-05-02T20:43:09Z

Well, yes. But in general I avoid using realpath as much as possible. node-sass has been using it to get the one true path of the node executable and we gave up sass/node-sass#1323.

kzc · 2016-05-02T20:46:14Z

Alright then, this appears to be a Windows libuv realpath issue, not a module resolution problem.

saper · 2016-05-02T20:47:31Z

No, that's not true. The fix in the module is incorrect. It aims at improving __dirname, it breaks things instead.

kzc · 2016-05-02T20:58:27Z

After the SUBST command is run, what is fs.realpathSync("c:/projects/node-6-native-ext-bug")? (assuming I got the Windows path format right)

Edit: In the new node 6.0.0 module resolution scheme the main script is still realpathed to get the right directory, but the other modules are not. This is why I think it's a libuv realpath issue if the expression above returns the wrong path.

saper · 2016-05-02T21:09:43Z

C:\Projects\node-6-native-ext-bug>v:\sw\node\Release\node -p fs.realpathSync('s:\\node-6-native-ext-bug')
C:\Projects\node-6-native-ext-bug

C:\Projects\node-6-native-ext-bug>v:\sw\node\Release\node -p fs.realpathSync('c:\\projects\\node-6-native-ext-bug')
C:\Projects\node-6-native-ext-bug

C:\Projects\node-6-native-ext-bug>subst
S:\: => C:\projects

So, realpath seems to work fine here.

kzc · 2016-05-02T21:14:04Z

Okay, there goes that theory.

If the same test is run with path.resolve instead of fs.realpathSync do you get the same result?

saper · 2016-05-02T21:18:46Z

C:\Projects\node-6-native-ext-bug>v:\sw\nodebrk -p path.resolve('s:\\node-6-native-ext-bug')
s:\node-6-native-ext-bug

C:\Projects\node-6-native-ext-bug>v:\sw\nodebrk -p path.resolve('c:\\projects\\node-6-native-ext-bug')
c:\projects\node-6-native-ext-bug

C:\Projects\node-6-native-ext-bug>subst
S:\: => C:\projects

kzc · 2016-05-02T21:23:20Z

I see. libuv handles SUBST correctly. Thanks.

kzc · 2016-05-02T21:31:31Z

Would it possible to get the value of require.resolve('buffertools') with node 6.0.0 when the command is run from C:\Projects\node-6-native-ext-bug and then from s:\node-6-native-ext-bug?

saper · 2016-05-02T21:45:07Z

C:\Projects\node-6-native-ext-bug>v:\sw\nodebrk.exe -p require.resolve('buffertools')
C:\Projects\node-6-native-ext-bug\node_modules\buffertools\buffertools.js

C:\Projects\node-6-native-ext-bug>s:

S:\>cd node-6-native-ext-bug

S:\node-6-native-ext-bug>v:\sw\nodebrk.exe -p require.resolve('buffertools')
S:\node-6-native-ext-bug\node_modules\buffertools\buffertools.js

saper · 2016-05-02T22:00:00Z

I think this problem is pretty easy to see from the npm test output:

Spawning: node S:\node-6-native-ext-bug\loader.js
Executing: C:\Projects\node-6-native-ext-bug\loader.js
Requiring: C:\Projects\node-6-native-ext-bug\node_modules\buffertools\build\Release\buffertools.node

The above lines run in a fresh copy of node (spawned) and have isMain true.
de1dc0a#diff-d1234a869b3d648ebfcdce5a76747d71R117 when isMain is true, we are using realpath to convert the path to the One True Path.

Then a module is included, and isMain ceases to be true now:

Requiring: S:\node-6-native-ext-bug\subloader.js
Executing: S:\node-6-native-ext-bug\subloader.js
Requiring: S:\node-6-native-ext-bug\node_modules\buffertools\build\Release\buffertools.node

Now we take another code path de1dc0a#diff-d1234a869b3d648ebfcdce5a76747d71R119 and we are not converting the path to the One True Path.

Therefore another attempt to load the binary module is undertook, which brings a whole thing down.

kzc · 2016-05-02T22:14:43Z

/cc @dlongley @lamara

MylesBorins · 2016-05-02T22:28:30Z

/cc @alexanderGugel

jasnell · 2016-05-02T23:48:22Z

See: #6537

dlongley · 2016-05-03T00:30:40Z

@saper,

Could you just add an additional require to get around the isMain issue?

dlongley · 2016-05-03T00:33:07Z

I'd be +1 to removing the isMain flag and never using realpath at all, by default. Not sure what the consequences of that would be, however.

saper · 2016-05-03T00:39:53Z

@dlongley node needs to use realpath not to try to load the same binary module twice. It's clunky, but that's how it currently works...

dlongley · 2016-05-03T00:44:42Z

@saper,

node needs to use realpath not to try to load the same binary module twice. It's clunky, but that's how it currently works...

Then we should only do realpath on binary modules.

kzc · 2016-05-03T02:48:04Z

@saper Earlier in this thread I asked if the same test case worked with node 6.0.0 with a symlink instead of SUBST. Just wanted to confirm that this is true or if I misinterpreted you. Just trying to determine the scope of the issue and whether symlinks are a workaround.

saper · 2016-05-03T17:30:20Z

I have not tried to reproduce it with symlinks before, I have only used SUBST.EXE.

But yesterday I wrote a test case for node to check for this issue and I have made the test case use MKLINK.EXE (the symbolic linking on Windows) if it gets administrative privileges. The symptoms are the same (it does not matter if one uses SUBST or MKLINK).

evanlucas · 2016-05-31T10:45:24Z

@xzyfer can you please try v6.2.0 and see if the problem still exists? Thanks!

This reverts commit 15fe42e to assert nodejs/node#6500 is resolved in Node v6.2.0.

xzyfer · 2016-06-07T09:38:34Z

@evanlucas I can confirm this is fixed in Node v6.2.0 in both my reduced reproduction and in the larger node-sass ci.

evanlucas · 2016-06-07T11:52:07Z

Glad to hear. Closing as this has been fixed. Thanks!!

This was originally removed in sass#1514 to work around nodejs/node#6500 which has since been resolved. This isn't playing nice with our npm cache in CI because we're changing the current path after running npm install.

xzyfer mentioned this issue May 1, 2016

module: preserve symlinks when requiring #5950

Closed

xzyfer mentioned this issue May 1, 2016

Update CI builds for NodeJS 6 sass/node-sass#1486

Closed

1 task

This was referenced May 1, 2016

Add Node v6.0.0 support sass/node-sass#1504

Closed

Node 6: Failed at the node-sass@3.6.0 postinstall script 'node scripts/build.js' sass/node-sass#1484

Closed

xzyfer changed the title ~~Possible Windows Node 6 regression with spawn and subset~~ Possible Windows Node 6 regression with spawn and a substed drive May 1, 2016

mscdex added child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform. labels May 1, 2016

saper mentioned this issue May 1, 2016

Node 6 for Windows: Don't use subst for testing sass/node-sass#1514

Merged

alexanderchan mentioned this issue May 2, 2016

Add Node v6.0.0 support webpack-contrib/sass-loader#241

Closed

saper mentioned this issue May 2, 2016

Don't resolve symlinks when requiring #3402

Closed

xzyfer mentioned this issue May 9, 2016

module: add --preserve-symlinks command line flag #6537

Closed

4 tasks

addaleax mentioned this issue May 18, 2016

Error: EISDIR illegal operation on directory - Binding.realpath #6861

Closed

xzyfer added a commit to sass/node-sass that referenced this issue Jun 7, 2016

Revert "Node 6 with AppVeyor: don't use subst for testing (#1514)"

aff8291

This reverts commit 15fe42e to assert nodejs/node#6500 is resolved in Node v6.2.0.

xzyfer mentioned this issue Jun 7, 2016

Revert "Node 6 with AppVeyor: don't use subst for testing (#1514)" sass/node-sass#1580

Closed

xzyfer added a commit to sass/node-sass that referenced this issue Jun 7, 2016

Revert "Node 6 with AppVeyor: don't use subst for testing (#1514)"

74010e1

This reverts commit 15fe42e to assert nodejs/node#6500 is resolved in Node v6.2.0.

xzyfer added a commit to sass/node-sass that referenced this issue Jun 7, 2016

Revert "Node 6 with AppVeyor: don't use subst for testing (#1514)"

998c9a1

This reverts commit 15fe42e to assert nodejs/node#6500 is resolved in Node v6.2.0.

evanlucas closed this as completed Jun 7, 2016

Possible Windows Node 6 regression with spawn and a substed drive #6500

Possible Windows Node 6 regression with spawn and a substed drive #6500

Comments

xzyfer commented May 1, 2016 • edited Loading

xzyfer commented May 1, 2016

saper commented May 1, 2016

saper commented May 1, 2016 • edited Loading

xzyfer commented May 1, 2016

saper commented May 2, 2016

xzyfer commented May 2, 2016

jasnell commented May 2, 2016

kzc commented May 2, 2016 • edited Loading

saper commented May 2, 2016

kzc commented May 2, 2016

saper commented May 2, 2016

kzc commented May 2, 2016 • edited Loading

saper commented May 2, 2016

kzc commented May 2, 2016

saper commented May 2, 2016

kzc commented May 2, 2016 • edited Loading

saper commented May 2, 2016 • edited Loading

kzc commented May 2, 2016

saper commented May 2, 2016

kzc commented May 2, 2016

kzc commented May 2, 2016 • edited Loading

saper commented May 2, 2016

saper commented May 2, 2016

kzc commented May 2, 2016

MylesBorins commented May 2, 2016 • edited Loading

jasnell commented May 2, 2016

dlongley commented May 3, 2016

dlongley commented May 3, 2016

saper commented May 3, 2016

dlongley commented May 3, 2016

kzc commented May 3, 2016

saper commented May 3, 2016 • edited Loading

evanlucas commented May 31, 2016

xzyfer commented Jun 7, 2016

evanlucas commented Jun 7, 2016

xzyfer commented May 1, 2016 •

edited

Loading

saper commented May 1, 2016 •

edited

Loading

kzc commented May 2, 2016 •

edited

Loading

kzc commented May 2, 2016 •

edited

Loading

kzc commented May 2, 2016 •

edited

Loading

saper commented May 2, 2016 •

edited

Loading

kzc commented May 2, 2016 •

edited

Loading

MylesBorins commented May 2, 2016 •

edited

Loading

saper commented May 3, 2016 •

edited

Loading