lib: include cached modules in module.children #14132

bnoordhuis · 2017-07-08T11:27:57Z

module.children is supposed to be the list of modules included by this
module but lib/module.js failed to update the list when the included
module was retrieved from Module._cache.

Fixes: #7131
CI: https://ci.nodejs.org/job/node-test-pull-request/9038/

bnoordhuis · 2017-07-08T11:30:33Z

test/fixtures/GH-7131/a.js

@@ -0,0 +1,5 @@
+'use strict';
+require('sys');  // Builtin should not show up in module.children array.


I picked a module that's unlikely to have already been included by test/common.js.

refack · 2017-07-08T19:27:59Z

Worth a CitGM run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/899/

Slayer95 · 2017-07-08T20:04:54Z

Does this mean that a structure a -> b -> a will be created if a and b are mutually dependent?

bnoordhuis · 2017-07-08T22:51:12Z

@Slayer95 Yes, there can be cycles now.

(That's the reason for the changes to test/sequential/test-module-loading.js: it didn't handle them.)

jasnell · 2017-07-10T16:33:48Z

test/parallel/test-module-children.js

+const common = require('../common');
+const assert = require('assert');
+
+const dir = common.fixturesDir + '/GH-7131';


path.join()?

Mwah, I can change that if you feel strongly but we have a gazillion tests that use simple string concatenation.

I'd prefer it. Updating the remaining ones for consistency would be a good Code-and-Learn activity (ping @nodejs/codeandlearn)

I'm not sure since https://nodejs.org/api/modules.html#modules_all_together (module names are URL like)
Maybe path.posix.join, but I'm +1 on literal /

@refack to be clear, you're saying that path.join() is wrong because it'll resolve to a\\b on Windows, and require always takes a POSIX style path (a/b)?

I'd rather have a concatenation (or `${common.fixturesDir}/GH-7131` ) than path.posix.join

@refack to be clear, you're saying that path.join() is wrong because it'll resolve to a\b on Windows, and require always takes a POSIX style path (a/b)?

Not wrong per se, just less right.

jasnell · 2017-07-10T16:34:05Z

test/parallel/test-module-children.js

+
+const dir = common.fixturesDir + '/GH-7131';
+const b = require(dir + '/b');
+const a = require(dir + '/a');


path.join()?

bnoordhuis · 2017-07-17T21:22:55Z

path.join() nits included, new CI: https://ci.nodejs.org/job/node-test-pull-request/9202/

refack · 2017-07-18T02:24:31Z

path.join() nits included, new CI: https://ci.nodejs.org/job/node-test-pull-request/9202/

I have to kill the ARM job https://ci.nodejs.org/job/node-test-binary-arm/9302/ It was Green (yellow) but wasn't reporting that it finished upstream.

jasnell · 2017-07-18T04:39:58Z

CI appears to be having some issues right now

bnoordhuis · 2017-07-18T10:58:44Z

CI again: https://ci.nodejs.org/job/node-test-pull-request/9216/

refack · 2017-07-18T11:09:38Z

CI again: https://ci.nodejs.org/job/node-test-pull-request/9216/

FWIW https://ci.nodejs.org/job/node-test-binary-arm/9302/ was green, except for a flake in freeBSD. And currently CI is even flakier then last night.

bnoordhuis · 2017-07-19T11:23:26Z

CI again: https://ci.nodejs.org/job/node-test-pull-request/9248/

bnoordhuis · 2017-07-19T11:24:47Z

FWIW https://ci.nodejs.org/job/node-test-binary-arm/9302/ was green, except for a flake in freeBSD. And currently CI is even flakier then last night.

@refack Are you saying that the whole run was green (except for a flake), not just the ARM machines? The node-test-binary-arm in the URL threw me off.

refack · 2017-07-19T12:29:15Z

@refack Are you saying that the whole run was green (except for a flake), not just the ARM machines? The node-test-binary-arm in the URL threw me off.

Yes. Whole run was green.

`module.children` is supposed to be the list of modules included by this module but lib/module.js failed to update the list when the included module was retrieved from `Module._cache`. Fixes: nodejs#7131 PR-URL: nodejs#14132 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

`module.children` is supposed to be the list of modules included by this module but lib/module.js failed to update the list when the included module was retrieved from `Module._cache`. Fixes: #7131 PR-URL: #14132 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

MylesBorins · 2017-08-16T07:57:08Z

@bnoordhuis should this be backported to LTS? if so how far back?

edit: considering this broke some stuff in the wild it might be best to just leave it out... at least of v4.x

Matchmaker is now globally accessible under Ladders, rather than needing to be manually required. This fixes a crash when hotpatching certain things.

Slayer95 · 2017-09-02T02:48:33Z

Confirming that this did break stuff at least for our project, which had to be dealt with using a global state-based quick&dirty fix.

MylesBorins · 2017-09-19T17:15:41Z

@nodejs/tsc
thoughts regarding the breakage mentioned above

MylesBorins · 2017-10-16T21:23:50Z

ping @nodejs/tsc and @nodejs/lts one more time

mcollina · 2017-10-16T21:27:15Z

-1 in backporting

kumarrishav · 2017-12-08T09:25:38Z

It's breaking this too. because of this . https://github.com/krakenjs/freshy/blob/master/index.js#L85

nodejs-github-bot added the module Issues and PRs related to the module subsystem. label Jul 8, 2017

bnoordhuis mentioned this pull request Jul 8, 2017

module.children doesn't contain cached modules #7131

Closed

bnoordhuis commented Jul 8, 2017

View reviewed changes

refack approved these changes Jul 8, 2017

View reviewed changes

cjihrig approved these changes Jul 9, 2017

View reviewed changes

tniessen approved these changes Jul 9, 2017

View reviewed changes

jasnell reviewed Jul 10, 2017

View reviewed changes

bnoordhuis force-pushed the fix7131 branch from 9a2e5e4 to 10827c2 Compare July 17, 2017 21:18

jasnell approved these changes Jul 17, 2017

View reviewed changes

refack approved these changes Jul 17, 2017

View reviewed changes

bnoordhuis force-pushed the fix7131 branch from 10827c2 to c83d9bb Compare July 24, 2017 15:00

bnoordhuis closed this Jul 24, 2017

bnoordhuis deleted the fix7131 branch July 24, 2017 15:00

bnoordhuis merged commit c83d9bb into nodejs:master Jul 24, 2017

addaleax mentioned this pull request Jul 24, 2017

v8.3.0 proposal #14452

Closed

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

mikermcneil mentioned this pull request Aug 15, 2017

Sails 1.0.0-33 doesn't boot on Node 8.3.0 when accessing require('sails').config balderdashy/sails#4165

Closed

MylesBorins added lts-watch-v4.x labels Aug 16, 2017

Slayer95 referenced this pull request in smogon/pokemon-showdown Sep 2, 2017

Fix Matchmaker hotpatching

7a9310a

Matchmaker is now globally accessible under Ladders, rather than needing to be manually required. This fixes a crash when hotpatching certain things.

MylesBorins added dont-land-on-v6.x and removed lts-watch-v4.x labels Oct 16, 2017

bnoordhuis mentioned this pull request May 2, 2019

JSON.stringify on process.nodeModules can result in an OOM when using custom serializer #27536

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: include cached modules in module.children #14132

lib: include cached modules in module.children #14132

bnoordhuis commented Jul 8, 2017 •

edited

Loading

bnoordhuis Jul 8, 2017

refack commented Jul 8, 2017

Slayer95 commented Jul 8, 2017

bnoordhuis commented Jul 8, 2017

jasnell Jul 10, 2017

bnoordhuis Jul 10, 2017

jasnell Jul 10, 2017

refack Jul 10, 2017

gibfahn Jul 11, 2017

refack Jul 12, 2017

jasnell Jul 10, 2017

bnoordhuis commented Jul 17, 2017

refack commented Jul 18, 2017

jasnell commented Jul 18, 2017

bnoordhuis commented Jul 18, 2017

refack commented Jul 18, 2017

bnoordhuis commented Jul 19, 2017

bnoordhuis commented Jul 19, 2017

refack commented Jul 19, 2017

MylesBorins commented Aug 16, 2017 •

edited

Loading

Slayer95 commented Sep 2, 2017 •

edited

Loading

MylesBorins commented Sep 19, 2017

MylesBorins commented Oct 16, 2017

mcollina commented Oct 16, 2017

kumarrishav commented Dec 8, 2017

		@@ -0,0 +1,5 @@
		'use strict';
		require('sys'); // Builtin should not show up in module.children array.

lib: include cached modules in module.children #14132

lib: include cached modules in module.children #14132

Conversation

bnoordhuis commented Jul 8, 2017 • edited Loading

Choose a reason for hiding this comment

refack commented Jul 8, 2017

Slayer95 commented Jul 8, 2017

bnoordhuis commented Jul 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Jul 17, 2017

refack commented Jul 18, 2017

jasnell commented Jul 18, 2017

bnoordhuis commented Jul 18, 2017

refack commented Jul 18, 2017

bnoordhuis commented Jul 19, 2017

bnoordhuis commented Jul 19, 2017

refack commented Jul 19, 2017

MylesBorins commented Aug 16, 2017 • edited Loading

Slayer95 commented Sep 2, 2017 • edited Loading

MylesBorins commented Sep 19, 2017

MylesBorins commented Oct 16, 2017

mcollina commented Oct 16, 2017

kumarrishav commented Dec 8, 2017

bnoordhuis commented Jul 8, 2017 •

edited

Loading

MylesBorins commented Aug 16, 2017 •

edited

Loading

Slayer95 commented Sep 2, 2017 •

edited

Loading