module: warn of potential for deadlock with hooks worker #51035

JakobJingleheimer · 2023-12-03T20:33:28Z

This issue was identified in #50948. Until we can provide a mitigation or proper solution, we should at least warn users of the danger.

nodejs-github-bot · 2023-12-03T20:33:33Z

Review requested:

@nodejs/loaders

github-actions · 2023-12-03T20:33:39Z

Fast-track has been requested by @JakobJingleheimer. Please 👍 to approve.

GeoffreyBooth · 2023-12-03T21:23:06Z

I feel like we should have a minimal reproduction before we document this.

It also doesn't need fast track, docs don't get updated until the next release so it doesn't matter much how quickly this lands.

JakobJingleheimer · 2023-12-03T21:37:03Z

We have a minimal repro.

RE fast-track: I guess; but the sooner it lands, the sooner it stops taking up my capacity.

doc/api/module.md

GeoffreyBooth · 2023-12-03T22:03:48Z

doc/api/module.md

+> example, you have 2 modules, A and B. "A" is registered first and sets up a
+> message channel,which it uses in its `resolve` hook. "B" uses `register` to
+> register its own loader. Resolving "B"'s own loader will go through "A"'s
+> `resolve`, which will try to communicate with the Module Worker. The Module


Suggested change

> `resolve`, which will try to communicate with the Module Worker. The Module

> `resolve`, which will try to communicate with the thread that the hooks are running on. The Module

No one knows what Module Worker means. I'm not even sure what it means.

Out of curiosity, who is "no one"?

GeoffreyBooth · 2023-12-03T22:04:13Z

doc/api/module.md

+> message channel,which it uses in its `resolve` hook. "B" uses `register` to
+> register its own loader. Resolving "B"'s own loader will go through "A"'s
+> `resolve`, which will try to communicate with the Module Worker. The Module
+> Worker is currently busy trying to register "B"'s loader, thus resulting in a


A Worker that the user created, or our hooks thread?

our hooks thread. I believe in node's source code, it is named module worker. Could be wrong—I haven't touched it in a while.

aduh95

If we do have a minimal repro (do we?), let's add it to test/known_issues

aduh95 · 2023-12-03T23:35:40Z

doc/api/module.md

+> **Warning** When setting up a `MessageChannel` to communicate with hooks,
+> beware that this can lead to a deadlock. For example, you have 2 modules,
+> A and B. "A" is registered first and sets up a message channel, which it uses
+> in its `resolve` hook. After "A" is registered, "B" is registered. Resolving
+> "B"'s specifier will go through "A"'s `resolve` hook, which will try to
+> communicate with a locked thread that is busy trying to register "B"'s hooks.
+> Since registering "B" depends on resolving "B"'s specifier, and resolving
+> "B"'s specifier is blocked by "A"'s communication request that is itself
+> blocked by the pending registration that started the chain, the application
+> becomes deadlocked.


I think this need a TLDR, in particular it needs an introduction sentence explaining what not to do – because let's be honest, I don't think anyone will be interested in the particular details unless they are running into that specific issue.
Maybe we can also tune down the details a lot, a vague explanation might be preferable.

TLDR: if there are multiple loaders and at least one uses MessageChannel, you will probably footgun

Could we keep it as that?

Suggested change

> **Warning** When setting up a `MessageChannel` to communicate with hooks,

> beware that this can lead to a deadlock. For example, you have 2 modules,

> A and B. "A" is registered first and sets up a message channel, which it uses

> in its `resolve` hook. After "A" is registered, "B" is registered. Resolving

> "B"'s specifier will go through "A"'s `resolve` hook, which will try to

> communicate with a locked thread that is busy trying to register "B"'s hooks.

> Since registering "B" depends on resolving "B"'s specifier, and resolving

> "B"'s specifier is blocked by "A"'s communication request that is itself

> blocked by the pending registration that started the chain, the application

> becomes deadlocked.

> **Warning** If a `resolve` or `load` is left pending on a response from a

> `MessageChannel`, that will cause a deadlock when the main thread is

> "asleep" waiting for a response from the loader thread. To avoid that,

> always set a timeout when dealing with cross thread communication

> inside those hooks.

waiting for a response from the hooks rather than loader thread. In this case just "hooks" is better than "hooks thread" because there could beer multiple hooks threads (for now).

To avoid that, always set a timeout when dealing with cross thread communication inside those hooks.

This is never going to be a suitable solution, it would be better not to suggest it.

Either the timeout will be too short, and will cancel requests that would not be deadlocked, or it's too long, and will incur a startup penalty equal to the length of the timeout when they are deadlocked. There's no reliable way to tune it to only cancel when it would be deadlocked, without significant perf penalties.

Furthermore, just canceling the request may actually not be what you want. Consider a transpiler loader that converts TypeScript into JavaScript, but has to talk to a service on the main thread to know how to do that correctly. If any Module.register() is called after this loader is registered, and the second loader is written in TypeScript, it's going to deadlock until the timer expires, then... what? Throw an error? Serve TypeScript to v8 uncompiled?

Yeah, I’d throw an error. After say one second of idling, it’s time to give up. It’s not a perf penalty, it’s a DX thing, querying the main thread is kind of like making a network call, you should always treat the case when you never get a response to not keep your users waiting indefinitely.

JakobJingleheimer · 2023-12-03T23:38:49Z

If we do have a minimal repro (do we?), let's add it to test/known_issues

Did not know this existed 😅 i'll add it tomorrow.

GeoffreyBooth · 2023-12-04T00:20:57Z

Does the issue require multiple user threads? Like can it happen regardless of whether the user has run new Worker?

isaacs · 2023-12-04T01:51:57Z

@GeoffreyBooth No new Worker involved. Just loaders being registered with Module.register(), and sending messages back to the main thread to decide how to return from the load/resolve hooks.

https://github.com/isaacs/node-21-import-deadlock

test/known_issues/hooks-thread/test-hooks-thread-deadlock.js

JakobJingleheimer · 2023-12-04T09:57:55Z

I added a test-case (which isn't quite complete for the issue reported: it still needs the piece on main to respond to the hooks-worker's request). But the incomplete test reveals that we actually have another problem first: a hook returning a never-settling promise causes a deadlock. @aduh95 I thought we specifically handled that in our original off-thread implementation? 🤔

aduh95 · 2023-12-04T11:09:29Z

I thought we specifically handled that in our original off-thread implementation? 🤔

Yep, we even have tests for that:

node/test/es-module/test-esm-loader-hooks.mjs

Line 53 in 4eafcf8

    
           describe('should handle never-settling hooks in ESM files', { concurrency: true }, () => {

I put this back to draft to avoid triggering test runs in CI (which will hang)

FYI a test that hasn't completed will timeout after 2 minutes:

node/tools/test.py

Lines 1364 to 1365 in 1b74aa3

    
           result.add_option("-t", "--timeout", help="Timeout in seconds", 
        
               default=120, type="int")

GeoffreyBooth · 2023-12-04T18:12:08Z

isaacs/node-21-import-deadlock

Could you reduce this into a minimal reproduction? Like something that could become a test in the Node codebase (assuming there’s a fix for it). Or maybe make a branch from the Node repo and use the existing fixtures to create a test that shows the issue.

test/known_issues/hooks-thread/a/hooks.mjs

test/known_issues/hooks-thread/test-hooks-thread-deadlock.js

aduh95 · 2023-12-04T18:32:13Z

test/known_issues/hooks-thread/test-hooks-thread-deadlock.js

+    let stderr = '';
+    let stdout = '';
+    // ! Do NOT use spawnSync here: it will deadlock.
+    const child = spawn(execPath, [


You want to write the test as you'd like Node.js to work, i.e. if we ever fix the bug, we should just have to git mv test/known_issues/test-hooks-deadlock.js test/es-module

Suggested change

const child = spawn(execPath, [

const result = await spawnPromisified(execPath, [

How do I then verify it is indeed "broken"? spawnPromisified will cause the process to hang.

I would call that broken, isn’t that good enough?

No, I mean it would break CI. The test as currently written demonstrates that the targeted behaviour is broken. Do these tests work differently?

Yeah, known_issues are for tests that are not passing (but we'd like them to). E.g. out/Release/node test/known_issues/test-vm-ownkeys.js exits with non-zero code, and tools/test.py test/known_issues/test-vm-ownkeys.js shows "All tests passed".

JakobJingleheimer · 2023-12-04T20:23:37Z

I thought we specifically handled that in our original off-thread implementation? 🤔

Yep, we even have tests for that:

How the heck is the hung promise in the test I've added here deadlocking then 😵

GeoffreyBooth · 2023-12-04T21:31:21Z

How the heck is the hung promise in the test I’ve added here deadlocking then 😵

Because of chaining?

test/known_issues/test-hooks-thread-deadlock.js

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

isaacs · 2023-12-09T22:25:32Z

Could you reduce this into a minimal reproduction? Like something that could become a test in the Node codebase (assuming there’s a fix for it). Or maybe make a branch from the Node repo and use the existing fixtures to create a test that shows the issue.

This is about as minimal as I could figure out how to make it. What would you suggest can be removed to make it simpler and still trigger the issue? It seems to require, at minimum:

two loaders
the first of which is using a MessageChannel and only returns from its async resolve hook after getting a response from the main thread
registered serially using Module.register() (either using the serialized --import args behavior that landed recently, or by explicitly loading both import scripts one after the other.)

GeoffreyBooth · 2023-12-11T03:01:16Z

What would you suggest can be removed to make it simpler and still trigger the issue?

I asked that before the PR with the failing test was created (or before I noticed it). I assume the failing test is about as minimal as we can get.

JakobJingleheimer added doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. fast-track PRs that do not need to wait for 48 hours to land. labels Dec 3, 2023

JakobJingleheimer marked this pull request as ready for review December 3, 2023 20:34

GeoffreyBooth reviewed Dec 3, 2023

View reviewed changes

JakobJingleheimer changed the title ~~module: warn of potential for deadlock with module worker~~ module: warn of potential for deadlock with hooks worker Dec 3, 2023

JakobJingleheimer added 3 commits December 3, 2023 23:59

module: warn of potential for deadlock with hooks worker

24490e8

wordsmith per PR feedback

c72fc75

remove extraneous details unnecessary for minimal repro

d9f61cb

JakobJingleheimer force-pushed the module/deadlock-warning branch from 984d6ce to d9f61cb Compare December 3, 2023 22:59

aduh95 removed the fast-track PRs that do not need to wait for 48 hours to land. label Dec 3, 2023

aduh95 reviewed Dec 3, 2023

View reviewed changes

JakobJingleheimer marked this pull request as draft December 4, 2023 09:01

JakobJingleheimer commented Dec 4, 2023

View reviewed changes

test/known_issues/hooks-thread/test-hooks-thread-deadlock.js Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

JakobJingleheimer force-pushed the module/deadlock-warning branch from 74e0826 to 60eb33f Compare December 4, 2023 09:54

test: add deadlock case

7068d97

JakobJingleheimer force-pushed the module/deadlock-warning branch from 60eb33f to 7068d97 Compare December 4, 2023 10:28

aduh95 reviewed Dec 4, 2023

View reviewed changes

test/known_issues/hooks-thread/a/hooks.mjs Outdated Show resolved Hide resolved

aduh95 reviewed Dec 4, 2023

View reviewed changes

test/known_issues/hooks-thread/test-hooks-thread-deadlock.js Outdated Show resolved Hide resolved

aduh95 reviewed Dec 4, 2023

View reviewed changes

chore: pathToFileURL

462d8f2

chore: move fixtures to junk-drawer

816cb01

aduh95 reviewed Dec 4, 2023

View reviewed changes

test/known_issues/test-hooks-thread-deadlock.js Outdated Show resolved Hide resolved

test/known_issues/test-hooks-thread-deadlock.js Outdated Show resolved Hide resolved

fixtures.fileURL

7c07922

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: warn of potential for deadlock with hooks worker #51035

module: warn of potential for deadlock with hooks worker #51035

JakobJingleheimer commented Dec 3, 2023

nodejs-github-bot commented Dec 3, 2023

github-actions bot commented Dec 3, 2023

GeoffreyBooth commented Dec 3, 2023

JakobJingleheimer commented Dec 3, 2023 •

edited

Loading

GeoffreyBooth Dec 3, 2023

JakobJingleheimer Dec 3, 2023

GeoffreyBooth Dec 3, 2023

JakobJingleheimer Dec 3, 2023 •

edited

Loading

aduh95 left a comment

aduh95 Dec 3, 2023 •

edited

Loading

JakobJingleheimer Dec 3, 2023

aduh95 Dec 3, 2023 •

edited

Loading

GeoffreyBooth Dec 4, 2023

isaacs Dec 4, 2023

aduh95 Dec 4, 2023

JakobJingleheimer commented Dec 3, 2023

GeoffreyBooth commented Dec 4, 2023

isaacs commented Dec 4, 2023

This comment was marked as outdated.

JakobJingleheimer commented Dec 4, 2023 •

edited

Loading

aduh95 commented Dec 4, 2023

GeoffreyBooth commented Dec 4, 2023 •

edited

Loading

aduh95 Dec 4, 2023 •

edited

Loading

JakobJingleheimer Dec 4, 2023

aduh95 Dec 4, 2023

JakobJingleheimer Dec 4, 2023

aduh95 Dec 4, 2023

JakobJingleheimer commented Dec 4, 2023

GeoffreyBooth commented Dec 4, 2023

isaacs commented Dec 9, 2023

GeoffreyBooth commented Dec 11, 2023

	> `resolve`, which will try to communicate with the Module Worker. The Module
	> `resolve`, which will try to communicate with the thread that the hooks are running on. The Module

	const child = spawn(execPath, [
	const result = await spawnPromisified(execPath, [

module: warn of potential for deadlock with hooks worker #51035

Are you sure you want to change the base?

module: warn of potential for deadlock with hooks worker #51035

Conversation

JakobJingleheimer commented Dec 3, 2023

nodejs-github-bot commented Dec 3, 2023

github-actions bot commented Dec 3, 2023

GeoffreyBooth commented Dec 3, 2023

JakobJingleheimer commented Dec 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobJingleheimer Dec 3, 2023 • edited Loading

Choose a reason for hiding this comment

aduh95 left a comment

Choose a reason for hiding this comment

aduh95 Dec 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduh95 Dec 3, 2023 • edited Loading

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

JakobJingleheimer commented Dec 3, 2023

GeoffreyBooth commented Dec 4, 2023

isaacs commented Dec 4, 2023

This comment was marked as outdated.

JakobJingleheimer commented Dec 4, 2023 • edited Loading

aduh95 commented Dec 4, 2023

GeoffreyBooth commented Dec 4, 2023 • edited Loading

aduh95 Dec 4, 2023 • edited Loading

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

JakobJingleheimer commented Dec 4, 2023

GeoffreyBooth commented Dec 4, 2023

isaacs commented Dec 9, 2023

GeoffreyBooth commented Dec 11, 2023

JakobJingleheimer commented Dec 3, 2023 •

edited

Loading

JakobJingleheimer Dec 3, 2023 •

edited

Loading

aduh95 Dec 3, 2023 •

edited

Loading

aduh95 Dec 3, 2023 •

edited

Loading

JakobJingleheimer commented Dec 4, 2023 •

edited

Loading

GeoffreyBooth commented Dec 4, 2023 •

edited

Loading

aduh95 Dec 4, 2023 •

edited

Loading