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.

Sign up for GitHub

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

Jump to bottom

ESM instrumentation/hooking fails with Node.js v18.19.0 #3784

Closed
david-luna opened this issue Dec 11, 2023 · 3 comments · Fixed by #3857
Closed

ESM instrumentation/hooking fails with Node.js v18.19.0 #3784

david-luna opened this issue Dec 11, 2023 · 3 comments · Fixed by #3857
Assignees
@trentm
Labels
agent-nodejs Make available for APM Agents project planning.

Comments

@david-luna
Copy link
Member

david-luna commented Dec 11, 2023 •
edited

Out CI stated failing recently in the test action only for node v18 on a specific test script tests/instrumentation/fixtures/use-knex-pg.mjs.
https://github.com/elastic/apm-agent-nodejs/actions/runs/7161350478/job/19496836227

The error reveals that the script is ran twice and a racing condition occurs when creating the database in postgres. The reason behind this double load seem to be a backport (from v20) of the loading process of modules.
nodejs/node#44710

We can reproduce with a small project

// package.json
{
  "name": "iitm-double-load",
  "version": "1.0.0",
  "author": "",
  "license": "ISC",
  "dependencies": {
    "import-in-the-middle": "^1.5.0"
  }
}

// file index.mjs
import { isMainThread } from 'worker_threads';

console.log('index module is loaded!!!', isMainThread);

You can see the module is loaded in the main and also in a worker thread

NODE_OPTIONS="--experimental-loader=import-in-the-middle/hook.mjs" NODE_NO_WARNINGS=1 node ./index.mjs
index module is loaded!!! false
index module is loaded!!! true
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Dec 11, 2023
@trentm trentm mentioned this issue Dec 11, 2023
Merged
trentm added a commit that referenced this issue Dec 11, 2023
@trentm
test: lock Node.js v18 testing to v18.18.2 as a workaround for IITM d…
a9dc49c 
…ouble-import issue (#3783)

In Nodejs v18.19.0 ESM module loading was moved to being off-thread
(a backport from Node.js v20, nodejs/node#44710).
This breaks current import-in-the-middle usage, resulting in ESM modules
being double-loaded when the IITM hook is active (once on the loader
worker thread, and then again later on the main thread).

This change is a workaround to skip testing recent Node.js v18
versions until the IITM issue is resolved.

Refs: #3784
@trentm
Copy link
Member

trentm commented Dec 11, 2023

workaround in ^^
The correct fix is an update of IITM with a fix.

@trentm
Copy link
Member

trentm commented Dec 11, 2023

IITM v1.6.0 was just released to fix this:

 '1.6.0': '2023-12-11T17:01:48.278Z'

So I guess we could have just waited an hour. :)

@trentm trentm self-assigned this Dec 11, 2023
@trentm trentm changed the title test failing on node v18.19.0 ESM instrumentation/hooking fails with Node.js v18.19.0 Dec 11, 2023
trentm added a commit that referenced this issue Dec 11, 2023
@trentm
fix: ESM hooking with Node.js v18.19.0
40490e8 
import-in-the-middle@1.6.0 fixes an issue with double-importing
with node 18.19.0 (due to a backport of ESM loading being done
off main thread).

Fixes: #3784
@trentm trentm mentioned this issue Dec 11, 2023
Closed
@david-luna
Copy link
Member Author

david-luna commented Dec 18, 2023 •
edited

IITM fix in DataDog/import-in-the-middle#44
Dependabot PR #3797

@david-luna david-luna mentioned this issue Dec 18, 2023
Closed
7 tasks
This was referenced Jan 11, 2024
Open
Merged
trentm added a commit that referenced this issue Jan 11, 2024
@trentm
test: lock Node.js v18 testing to v18.18.2 as a workaround for IITM d…
730fc8f 
…ouble-import issue (#3783)

In Nodejs v18.19.0 ESM module loading was moved to being off-thread
(a backport from Node.js v20, nodejs/node#44710).
This breaks current import-in-the-middle usage, resulting in ESM modules
being double-loaded when the IITM hook is active (once on the loader
worker thread, and then again later on the main thread).

This change is a workaround to skip testing recent Node.js v18
versions until the IITM issue is resolved.

Refs: #3784
@trentm trentm mentioned this issue Jan 11, 2024
Merged
trentm added a commit that referenced this issue Jan 11, 2024
@trentm
test: lock Node.js v18 testing to v18.18.2 as a workaround for IITM d…
82dfd2e 
…ouble-import issue (#3824)

(backport of #3783)

In Nodejs v18.19.0 ESM module loading was moved to being off-thread
(a backport from Node.js v20, nodejs/node#44710).
This breaks current import-in-the-middle usage, resulting in ESM modules
being double-loaded when the IITM hook is active (once on the loader
worker thread, and then again later on the main thread).

This change is a workaround to skip testing recent Node.js v18
versions until the IITM issue is resolved.

Refs: #3784
@trentm trentm mentioned this issue Feb 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@trentm trentm

Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Milestone
No milestone
2 participants
@trentm @david-luna