module: throw error when re-runing errored module jobs · nodejs/node@4acf7cd · GitHub

nodejs / node Public

• Notifications You must be signed in to change notification settings
• Fork 36.6k
• Star 120k

• Code
• Issues 632
• Pull requests 748
• Actions
• Projects
• Security and quality
• Insights

Additional navigation options

Commit 4acf7cd

Browse filesBrowse the repository at this point in the historyBrowse files

joyeecheung

authored and

aduh95

committed

module: throw error when re-runing errored module jobs

Re-evaluating an errored ESM should lead to rejecting the rejection again - this is also the case when importing it twice. In the case of retrying with require after import, just throw the cached error. Drive-by: add some debug logs. PR-URL: #58957 Fixes: #58945 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

1 parent bace73a commit 4acf7cd
Copy full SHA for 4acf7cd

4 files changed

+47-2Lines changed: 47 additions & 2 deletions

File tree

Expand file treeCollapse file tree

Open diff view settings

Filter options

• lib/internal/modules/esm
  • loader.js
  • module_job.js
• test/es-module
  • test-import-module-retry-require-errored.js
  • test-require-module-retry-import-errored-2.js

Expand file treeCollapse file tree

Open diff view settings

Collapse file

‎lib/internal/modules/esm/loader.js‎

Copy file name to clipboardExpand all lines: lib/internal/modules/esm/loader.js

+4Lines changed: 4 additions & 0 deletions

Original file line number	Diff line number	Diff line change
@@ -42,6 +42,7 @@ const {
42	42	kEvaluated,
43	43	kEvaluating,
44	44	kInstantiated,
	45	+ kErrored,
45	46	throwIfPromiseRejected,
46	47	} = internalBinding('module_wrap');
47	48	const {
@@ -394,6 +395,9 @@ class ModuleLoader {
394	395	mod[kRequiredModuleSymbol] = job.module;
395	396	const { namespace } = job.runSync(parent);
396	397	return { wrap: job.module, namespace: namespace || job.module.getNamespace() };
	398	+ } else if (status === kErrored) {
	399	+ // If the module was previously imported and errored, throw the error.
	400	+ throw job.module.getError();
397	401	}
398	402	// When the cached async job have already encountered a linking
399	403	// error that gets wrapped into a rejection, but is still later

Collapse file

‎lib/internal/modules/esm/module_job.js‎

Copy file name to clipboardExpand all lines: lib/internal/modules/esm/module_job.js

+10-2Lines changed: 10 additions & 2 deletions

Original file line number	Diff line number	Diff line change
@@ -297,7 +297,7 @@ class ModuleJob extends ModuleJobBase {
297	297	assert(this.module instanceof ModuleWrap);
298	298	let status = this.module.getStatus();
299	299
300		- debug('ModuleJob.runSync', this.module);
	300	+ debug('ModuleJob.runSync()', status, this.module);
301	301	// FIXME(joyeecheung): this cannot fully handle < kInstantiated. Make the linking
302	302	// fully synchronous instead.
303	303	if (status === kUninstantiated) {
@@ -332,6 +332,8 @@ class ModuleJob extends ModuleJobBase {
332	332	}
333	333
334	334	async run(isEntryPoint = false) {
	335	+ debug('ModuleJob.run()', this.module);
	336	+ assert(this.phase === kEvaluationPhase);
335	337	await this.instantiate();
336	338	if (isEntryPoint) {
337	339	globalThis[entry_point_module_private_symbol] = this.module;
@@ -411,7 +413,11 @@ class ModuleJobSync extends ModuleJobBase {
411	413	async run() {
412	414	// This path is hit by a require'd module that is imported again.
413	415	const status = this.module.getStatus();
414		- if (status > kInstantiated) {
	416	+ debug('ModuleJobSync.run()', status, this.module);
	417	+ // If the module was previously required and errored, reject from import() again.
	418	+ if (status === kErrored) {
	419	+ throw this.module.getError();
	420	+ } else if (status > kInstantiated) {
415	421	if (this.evaluationPromise) {
416	422	await this.evaluationPromise;
417	423	}
@@ -432,6 +438,8 @@ class ModuleJobSync extends ModuleJobBase {
432	438	}
433	439
434	440	runSync(parent) {
	441	+ debug('ModuleJobSync.runSync()', this.module);
	442	+ assert(this.phase === kEvaluationPhase);
435	443	// TODO(joyeecheung): add the error decoration logic from the async instantiate.
436	444	this.module.async = this.module.instantiateSync();
437	445	// If --experimental-print-required-tla is true, proceeds to evaluation even

Collapse file

‎test/es-module/test-import-module-retry-require-errored.js‎

Copy file name to clipboard

+17Lines changed: 17 additions & 0 deletions

Original file line number	Diff line number	Diff line change
@@ -0,0 +1,17 @@
	1	+ // This tests that after failing to import an ESM that rejects,
	2	+ // retrying with require() still throws.
	3	+
	4	+ 'use strict';
	5	+ const common = require('../common');
	6	+ const assert = require('assert');
	7	+
	8	+ (async () => {
	9	+ await assert.rejects(import('../fixtures/es-modules/throw-error.mjs'), {
	10	+ message: 'test',
	11	+ });
	12	+ assert.throws(() => {
	13	+ require('../fixtures/es-modules/throw-error.mjs');
	14	+ }, {
	15	+ message: 'test',
	16	+ });
	17	+ })().then(common.mustCall());

Collapse file

‎test/es-module/test-require-module-retry-import-errored-2.js‎

Copy file name to clipboard

+16Lines changed: 16 additions & 0 deletions

Original file line number	Diff line number	Diff line change
@@ -0,0 +1,16 @@
	1	+ // This tests that after failing to require an ESM that throws,
	2	+ // retrying with import() still rejects.
	3	+
	4	+ 'use strict';
	5	+ const common = require('../common');
	6	+ const assert = require('assert');
	7	+
	8	+ assert.throws(() => {
	9	+ require('../fixtures/es-modules/throw-error.mjs');
	10	+ }, {
	11	+ message: 'test',
	12	+ });
	13	+
	14	+ assert.rejects(import('../fixtures/es-modules/throw-error.mjs'), {
	15	+ message: 'test',
	16	+ }).then(common.mustCall());

0 commit comments

Comments

0 (0)