Skip to content
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

Repro of Node loading modules twice on Windows #6978

Closed
Jeff-Lewis opened this issue May 25, 2016 · 9 comments
Closed

Repro of Node loading modules twice on Windows #6978

Jeff-Lewis opened this issue May 25, 2016 · 9 comments
Assignees
Labels
module Issues and PRs related to the module subsystem. windows Issues and PRs related to the Windows platform.

Comments

@Jeff-Lewis
Copy link

Jeff-Lewis commented May 25, 2016

Branching issue #6624

Node loading a module twice on Windows

Problem:

Nodejs modules can be loaded twice on Windows due to a change in the drive letter casing.

Using only relative paths, this repo shows how the drive letter can change using require().

Uppercase require.resolve('./moduleA') C:\temp\node_6624\moduleA.js

changes to

Lowercase require.resolve('./../moduleA') c:\temp\node_6624\moduleA.js

Demonstration of how node modules can be loaded twice on Windows under the following circumstances:
  1. command line launching initial node module must use uppercase drive letter, ex. C:\server.js
  2. There must be a circular reference between modules somewhere along the dependency chain where
    a child module (in this example moduleB) must be require()'ed with lowercase drive letter, ex. require(path.join('dir','moduleB'))
  3. When moduleB requires moduleA, it will use the lowercase drive letter in it's path.

A dirty diagram...
C:\moduleA -> c:\moduleB -> c:\moduleA

To Reproduce:
  1. Using Windows, clone this repo
  2. run run.bat

You will see at the end of the console output, moduleA.js was loaded twice.

*** require.cache ***
[ 'C:\\temp\\node_6624\\test.js',
  'C:\\temp\\node_6624\\moduleA.js',
  'c:\\temp\\node_6624\\childFolder\\moduleB.js',
  'c:\\temp\\node_6624\\moduleA.js' ]

See test.js for code

How often does this happen?
  1. WebStorm IDE by default uses uppercase letters in the command line when debugging node.
  2. Loading modules by way of path.join() is common in many frameworks.
Why should this be fixed?

The case of the drive letter on Windows should not affect the node run-time and how modules are loaded. _Once this case occurs, all require() paths will be changed causing node to potentially load ALL subsequent modules again._

References:

#6624
#6624 (comment)

  • Version: 4.4
  • Platform: Windows 10 64-bit
  • Subsystem: none
@addaleax addaleax added module Issues and PRs related to the module subsystem. windows Issues and PRs related to the Windows platform. labels May 25, 2016
@orangemocha orangemocha self-assigned this May 26, 2016
@orangemocha
Copy link
Contributor

I'll revive nodejs/node-v0.x-archive#6774, which I think is an appropriate fix for this issue.

orangemocha added a commit to JaneaSystems/node that referenced this issue May 27, 2016
Module cache lookups should be case-insensitive on Windows, or
invoking require() with different casings will end up loading multiple
instances of a module.

Fixes: nodejs#6978
@luisrudge
Copy link

I'm having this issue as well and is specially annoying using front end libs because you load two of them (React, for example) and everything blows up

@Jeff-Lewis
Copy link
Author

Awesome @orangemocha. Is more needed for a PR?

@orangemocha
Copy link
Contributor

I am investigating a unit test failure with those changes. It should be ready shortly.

@Jeff-Lewis
Copy link
Author

Hi @orangemocha. How is the testing going? This issue bit be again in another project today.

@orangemocha
Copy link
Contributor

I think I have all the pieces of the puzzle now:

  • The test failures were because of require.cache exposes the cache key and with my change I changed the case of it. I think the solution is to make require.resolve also normalize the case and update the documentation to clarify that require.cache should always be keyed with the result of require.resolve
  • I tracked down why the keys currently always have an uppercase drive letter. It is because the full path is resolved based on the requiring module __filename, and that is calculated using fs.realPath. That in turn uses GetFinalPathNameByHandleW in libuv, which performs the normalization even if I pass in FILE_NAME_OPENED.

I think the cache key normalization change can be made regardless of the __filename drive normalization, but I wanted to make sure I understood the whole picture.

I am on vacation now and unfortunately I didn't have enough time to open the PR and defend it before leaving. I am going to resume this as soon as I get back on July 1st.

@Jeff-Lewis
Copy link
Author

ref #7175 for another discussion on changes to fs.realPath

@bruno-brant
Copy link

bruno-brant commented Jul 24, 2016

I just had my issues with this: I'm connecting mongoose in one place, only to have it disconnected in another. Tracking this down took a lot of time too.

Let's just hope you guys can solve this soon.

One other thing: I don't think the scenario is the one stated by @Jeff-Lewis , instead, a much broader one - at least in my case, using vscode.

The thing is that the first module loaded by node "inherits" the drive letter casing from the command line, and everything it requires ends up with the same casing. However, indirect dependencies, the ones required by the modules that the main module loads, ends up having lower case drive letter.

If this is true, the work around would be simple, just adding a proxy main module that delegates to the actual one.

I will properly test this hypothesis later and will come back with results.

Tried to confirm my hypothesis, no luck.

@orangemocha
Copy link
Contributor

The real cause of this regression is documented at #7726

bzoz added a commit to JaneaSystems/node that referenced this issue Aug 5, 2016
This reverts parts of nodejs@b488b19
restoring javascript implementation of realpath and realpathSync.

Fixes: nodejs#7175
Fixes: nodejs#6861
Fixes: nodejs#7294
Fixes: nodejs#7192
Fixes: nodejs#7044
Fixes: nodejs#6624
Fixes: nodejs#6978
@bzoz bzoz closed this as completed in 08996fd Aug 12, 2016
cjihrig pushed a commit that referenced this issue Aug 15, 2016
This reverts parts of b488b19
restoring javascript implementation of realpath and realpathSync.

Fixes: #7175
Fixes: #6861
Fixes: #7294
Fixes: #7192
Fixes: #7044
Fixes: #6624
Fixes: #6978
PR-URL: #7899
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

5 participants