Permalink
Browse files

src,module: add --preserve-symlinks command line flag

Add the `--preserve-symlinks` flag. This makes the changes added
in #5950 conditional. By default the old behavior is used. With
the flag set, symlinks are preserved, switching to the new
behavior. This should be considered to be a temporary solution
until we figure out how to solve the symlinked peer dependency
problem in a more general way that does not break everything
else.

Additional test cases are included.

PR-URL: #6537
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
  • Loading branch information...
1 parent f52b2f1 commit 5d38d543cdd25962fadc49a997798d156a41e4c7 @jasnell jasnell committed May 2, 2016
View
@@ -97,6 +97,43 @@ Automatically zero-fills all newly allocated [Buffer][] and [SlowBuffer][]
instances.
+### `--preserve-symlinks`
+
+Instructs the module loader to preserve symbolic links when resolving and
+caching modules.
+
+By default, when Node.js loads a module from a path that is symbolically linked
+to a different on-disk location, Node.js will dereference the link and use the
+actual on-disk "real path" of the module as both an identifier and as a root
+path to locate other dependency modules. In most cases, this default behavior
+is acceptable. However, when using symbolically linked peer dependencies, as
+illustrated in the example below, the default behavior causes an exception to
+be thrown if `moduleA` attempts to require `moduleB` as a peer dependency:
+
+```text
+{appDir}
+ ├── app
+ │ ├── index.js
+ │ └── node_modules
+ │ ├── moduleA -> {appDir}/moduleA
+ │ └── moduleB
+ │ ├── index.js
+ │ └── package.json
+ └── moduleA
+ ├── index.js
+ └── package.json
+```
+
+The `--preserve-symlinks` command line flag instructs Node.js to use the
+symlink path for modules as opposed to the real path, allowing symbolically
+linked peer dependencies to be found.
+
+Note, however, that using `--preserve-symlinks` can have other side effects.
+Specifically, symbolically linked *native* modules can fail to load if those
+are linked from more than one location in the dependency tree (Node.js would
+see those as two separate modules and would attempt to load the module multiple
+times, causing an exception to be thrown).
+
### `--track-heap-objects`
Track heap object allocations for heap snapshots.
@@ -138,7 +175,6 @@ Force FIPS-compliant crypto on startup. (Cannot be disabled from script code.)
Specify ICU data load path. (overrides `NODE_ICU_DATA`)
-
## Environment Variables
### `NODE_DEBUG=module[,…]`
View
@@ -96,6 +96,11 @@ of the event loop.
Automatically zero-fills all newly allocated Buffer and SlowBuffer instances.
.TP
+.BR \-\-preserve\-symlinks
+Instructs the module loader to preserve symbolic links when resolving and
+caching modules.
+
+.TP
.BR \-\-track\-heap-objects
Track heap object allocations for heap snapshots.
View
@@ -10,6 +10,7 @@ const fs = require('fs');
const path = require('path');
const internalModuleReadFile = process.binding('fs').internalModuleReadFile;
const internalModuleStat = process.binding('fs').internalModuleStat;
+const preserveSymlinks = !!process.binding('config').preserveSymlinks;
// If obj.hasOwnProperty has been overridden, then calling
// obj.hasOwnProperty(prop) will break.
@@ -109,14 +110,15 @@ function tryPackage(requestPath, exts, isMain) {
}
// check if the file exists and is not a directory
-// resolve to the absolute realpath if running main module,
-// otherwise resolve to absolute while keeping symlinks intact.
+// if using --preserve-symlinks and isMain is false,
+// keep symlinks intact, otherwise resolve to the
+// absolute realpath.
function tryFile(requestPath, isMain) {
const rc = stat(requestPath);
- if (isMain) {
- return rc === 0 && fs.realpathSync(requestPath);
+ if (preserveSymlinks && !isMain) {
+ return rc === 0 && path.resolve(requestPath);
}
- return rc === 0 && path.resolve(requestPath);
+ return rc === 0 && fs.realpathSync(requestPath);
}
// given a path check a the file exists with any of the set extensions
@@ -159,7 +161,7 @@ Module._findPath = function(request, paths, isMain) {
if (!trailingSlash) {
const rc = stat(basePath);
if (rc === 0) { // File.
- if (!isMain) {
+ if (preserveSymlinks && !isMain) {
filename = path.resolve(basePath);
} else {
filename = fs.realpathSync(basePath);
View
@@ -168,6 +168,11 @@ bool force_fips_crypto = false;
bool no_process_warnings = false;
bool trace_warnings = false;
+// Set in node.cc by ParseArgs when --preserve-symlinks is used.
+// Used in node_config.cc to set a constant on process.binding('config')
+// that is used by lib/module.js
+bool config_preserve_symlinks = false;
+
// process-relative uptime base, initialized at start-up
static double prog_start_time;
static bool debugger_running;
@@ -3460,6 +3465,8 @@ static void PrintHelp() {
" note: linked-in ICU data is\n"
" present.\n"
#endif
+ " --preserve-symlinks preserve symbolic links when resolving\n"
+ " and caching modules.\n"
#endif
"\n"
"Environment variables:\n"
@@ -3589,6 +3596,8 @@ static void ParseArgs(int* argc,
} else if (strncmp(arg, "--security-revert=", 18) == 0) {
const char* cve = arg + 18;
Revert(cve);
+ } else if (strcmp(arg, "--preserve-symlinks") == 0) {
+ config_preserve_symlinks = true;
} else if (strcmp(arg, "--prof-process") == 0) {
prof_process = true;
short_circuit = true;
View
@@ -41,7 +41,10 @@ void InitConfig(Local<Object> target,
if (flag_icu_data_dir)
READONLY_BOOLEAN_PROPERTY("usingICUDataDir");
#endif // NODE_HAVE_I18N_SUPPORT
-}
+
+ if (config_preserve_symlinks)
+ READONLY_BOOLEAN_PROPERTY("preserveSymlinks");
+} // InitConfig
} // namespace node
@@ -30,6 +30,11 @@ struct sockaddr;
namespace node {
+// Set in node.cc by ParseArgs when --preserve-symlinks is used.
+// Used in node_config.cc to set a constant on process.binding('config')
+// that is used by lib/module.js
+extern bool config_preserve_symlinks;
+
// Forward declaration
class Environment;
@@ -0,0 +1,13 @@
+#include <node.h>
+#include <v8.h>
+
+void Method(const v8::FunctionCallbackInfo<v8::Value>& args) {
+ v8::Isolate* isolate = args.GetIsolate();
+ args.GetReturnValue().Set(v8::String::NewFromUtf8(isolate, "world"));
+}
+
+void init(v8::Local<v8::Object> target) {
+ NODE_SET_METHOD(target, "hello", Method);
+}
+
+NODE_MODULE(binding, init);
@@ -0,0 +1,8 @@
+{
+ 'targets': [
+ {
+ 'target_name': 'binding',
+ 'sources': [ 'binding.cc' ]
+ }
+ ]
+}
@@ -0,0 +1,13 @@
+'use strict';
+require('../../common');
+const path = require('path');
+const assert = require('assert');
+
+// This is a subtest of symlinked-module/test.js. This is not
+// intended to be run directly.
+
+module.exports.test = function test(bindingDir) {
+ const mod = require(path.join(bindingDir, 'binding.node'));
+ assert.notStrictEqual(mod, null);
+ assert.strictEqual(mod.hello(), 'world');
+};
@@ -0,0 +1,34 @@
+'use strict';
+const common = require('../../common');
+const fs = require('fs');
+const path = require('path');
+const assert = require('assert');
+
+// This test verifies that symlinked native modules can be required multiple
+// times without error. The symlinked module and the non-symlinked module
+// should be the same instance. This expectation was not previously being
+// tested and ended up being broken by https://github.com/nodejs/node/pull/5950.
+
+// This test should pass in Node.js v4 and v5. This test will pass in Node.js
+// with https://github.com/nodejs/node/pull/5950 reverted.
+
+common.refreshTmpDir();
+
+const addonPath = path.join(__dirname, 'build', 'Release');
+const addonLink = path.join(common.tmpDir, 'addon');
+
+try {
+ fs.symlinkSync(addonPath, addonLink);
+} catch (err) {
+ if (err.code !== 'EPERM') throw err;
+ common.skip('module identity test (no privs for symlinks)');
+ return;
+}
+
+const sub = require('./submodule');
+[addonPath, addonLink].forEach((i) => {
+ const mod = require(path.join(i, 'binding.node'));
+ assert.notStrictEqual(mod, null);
+ assert.strictEqual(mod.hello(), 'world');
+ assert.doesNotThrow(() => sub.test(i));
+});
@@ -0,0 +1,68 @@
+'use strict';
+
+// This tests to make sure that modules with symlinked circular dependencies
+// do not blow out the module cache and recurse forever. See issue
+// https://github.com/nodejs/node/pull/5950 for context. PR #5950 attempted
+// to solve a problem with symlinked peer dependencies by caching using the
+// symlink path. Unfortunately, that breaks the case tested in this module
+// because each symlinked module, despite pointing to the same code on disk,
+// is loaded and cached as a separate module instance, which blows up the
+// cache and leads to a recursion bug.
+
+// This test should pass in Node.js v4 and v5. It should pass in Node.js v6
+// after https://github.com/nodejs/node/pull/5950 has been reverted.
+
+const common = require('../common');
+const assert = require('assert');
+const path = require('path');
+const fs = require('fs');
+
+// {tmpDir}
+// ├── index.js
+// └── node_modules
+// ├── moduleA
+// │ ├── index.js
+// │ └── node_modules
+// │ └── moduleB -> {tmpDir}/node_modules/moduleB
+// └── moduleB
+// ├── index.js
+// └── node_modules
+// └── moduleA -> {tmpDir}/node_modules/moduleA
+
+common.refreshTmpDir();
+const tmpDir = common.tmpDir;
+
+const node_modules = path.join(tmpDir, 'node_modules');
+const moduleA = path.join(node_modules, 'moduleA');
+const moduleB = path.join(node_modules, 'moduleB');
+const moduleA_link = path.join(moduleB, 'node_modules', 'moduleA');
+const moduleB_link = path.join(moduleA, 'node_modules', 'moduleB');
+
+fs.mkdirSync(node_modules);
+fs.mkdirSync(moduleA);
+fs.mkdirSync(moduleB);
+fs.mkdirSync(path.join(moduleA, 'node_modules'));
+fs.mkdirSync(path.join(moduleB, 'node_modules'));
+
+try {
+ fs.symlinkSync(moduleA, moduleA_link);
+ fs.symlinkSync(moduleB, moduleB_link);
+} catch (err) {
+ if (err.code !== 'EPERM') throw err;
+ common.skip('insufficient privileges for symlinks');
+ return;
+}
+
+fs.writeFileSync(path.join(tmpDir, 'index.js'),
+ 'module.exports = require(\'moduleA\');', 'utf8');
+fs.writeFileSync(path.join(moduleA, 'index.js'),
+ 'module.exports = {b: require(\'moduleB\')};', 'utf8');
+fs.writeFileSync(path.join(moduleB, 'index.js'),
+ 'module.exports = {a: require(\'moduleA\')};', 'utf8');
+
+// Ensure that the symlinks are not followed forever...
+const obj = require(path.join(tmpDir, 'index'));
+assert.ok(obj);
+assert.ok(obj.b);
+assert.ok(obj.b.a);
+assert.ok(!obj.b.a.b);
@@ -0,0 +1,65 @@
+// Flags: --preserve-symlinks
+'use strict';
+// Refs: https://github.com/nodejs/node/pull/5950
+
+// This test verifies that symlinked modules are able to find their peer
+// dependencies when using the --preserve-symlinks command line flag.
+
+// This test passes in v6.2+ with --preserve-symlinks on and in v6.0/v6.1.
+// This test will fail in Node.js v4 and v5 and should not be backported.
+
+const common = require('../common');
+const fs = require('fs');
+const path = require('path');
+const assert = require('assert');
+
+common.refreshTmpDir();
+
+const tmpDir = common.tmpDir;
+
+// Creates the following structure
+// {tmpDir}
+// ├── app
+// │ ├── index.js
+// │ └── node_modules
+// │ ├── moduleA -> {tmpDir}/moduleA
+// │ └── moduleB
+// │ ├── index.js
+// │ └── package.json
+// └── moduleA
+// ├── index.js
+// └── package.json
+
+const moduleA = path.join(tmpDir, 'moduleA');
+const app = path.join(tmpDir, 'app');
+const moduleB = path.join(app, 'node_modules', 'moduleB');
+const moduleA_link = path.join(app, 'node_modules', 'moduleA');
+fs.mkdirSync(moduleA);
+fs.mkdirSync(app);
+fs.mkdirSync(path.join(app, 'node_modules'));
+fs.mkdirSync(moduleB);
+
+// Attempt to make the symlink. If this fails due to lack of sufficient
+// permissions, the test will bail out and be skipped.
+try {
+ fs.symlinkSync(moduleA, moduleA_link);
+} catch (err) {
+ if (err.code !== 'EPERM') throw err;
+ common.skip('insufficient privileges for symlinks');
+ return;
+}
+
+fs.writeFileSync(path.join(moduleA, 'package.json'),
+ JSON.stringify({name: 'moduleA', main: 'index.js'}), 'utf8');
+fs.writeFileSync(path.join(moduleA, 'index.js'),
+ 'module.exports = require(\'moduleB\');', 'utf8');
+fs.writeFileSync(path.join(app, 'index.js'),
+ '\'use strict\'; require(\'moduleA\');', 'utf8');
+fs.writeFileSync(path.join(moduleB, 'package.json'),
+ JSON.stringify({name: 'moduleB', main: 'index.js'}), 'utf8');
+fs.writeFileSync(path.join(moduleB, 'index.js'),
+ 'module.exports = 1;', 'utf8');
+
+assert.doesNotThrow(() => {
+ require(path.join(app, 'index'));
+});
@@ -1,3 +1,4 @@
+// Flags: --preserve-symlinks
'use strict';
const common = require('../common');
const assert = require('assert');
@@ -49,7 +50,7 @@ function test() {
// load symlinked-script as main
var node = process.execPath;
- var child = spawn(node, [linkScript]);
+ var child = spawn(node, ['--preserve-symlinks', linkScript]);
child.on('close', function(code, signal) {
assert(!code);
assert(!signal);

0 comments on commit 5d38d54

Please sign in to comment.