nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.4k stars 28.99k forks source link

bug: segmentation fault in REPL when trying to use AbortController with node-fetch package, dynamic import and timeouts #44464

Open HarikrishnanBalagopal opened 2 years ago

HarikrishnanBalagopal commented 2 years ago

Version

v16.17.0

Platform

Darwin ibm-macbook-pro 21.6.0 Darwin Kernel Version 21.6.0: Wed Aug 10 14:25:27 PDT 2022; root:xnu-8020.141.5~2/RELEASE_X86_64 x86_64 i386 MacBookPro16,1 Darwin

Subsystem

REPL, dynamic import, abort controller, timeout

What steps will reproduce the bug?

$ mkdir segfault
$ cd segfault/
$ npm init -y
Wrote to /Users/user/temp/segfault/package.json:

{
  "name": "segfault",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "keywords": [],
  "author": "",
  "license": "ISC"
}

$ npm install node-fetch

added 6 packages, and audited 7 packages in 3s

3 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
$ node
Welcome to Node.js v16.17.0.
Type ".help" for more information.
> const fetch = (...args) => import('node-fetch').then(({default: fetch}) => fetch(...args));
undefined
> async function fetchWithTimeout(resource, options = {}) {
...   const { timeout = 8000 } = options;
...   
...   const controller = new AbortController();
...   const id = setTimeout(() => controller.abort(), timeout);
...   const response = await fetch(resource, {
...     ...options,
...     signal: controller.signal  
...   });
...   clearTimeout(id);
...   return response;
... }
undefined
> async function loadGames(x) {
...   try {
...     const response = await fetchWithTimeout('https://www.google.com/games', {
...       timeout: x,
...     });
...     const games = await response.json();
...     return games;
...   } catch (error) {
...     // Timeouts if the request takes
...     // longer than 6 seconds
...     console.log(error.name === 'AbortError');
...   }
... }
undefined
> await loadGames(10);
Segmentation fault: 11

You can copy paste the following code into the REPL to trigger the segmentation fault

const fetch = (...args) => import('node-fetch').then(({default: fetch}) => fetch(...args));
async function fetchWithTimeout(resource, options = {}) {
  const { timeout = 8000 } = options;

  const controller = new AbortController();
  const id = setTimeout(() => controller.abort(), timeout);
  const response = await fetch(resource, {
    ...options,
    signal: controller.signal  
  });
  clearTimeout(id);
  return response;
}
async function loadGames(x) {
  try {
    const response = await fetchWithTimeout('https://www.google.com/games', {
      timeout: x,
    });
    const games = await response.json();
    return games;
  } catch (error) {
    // Timeouts if the request takes
    // longer than 6 seconds
    console.log(error.name === 'AbortError');
  }
}
await loadGames(10);

How often does it reproduce? Is there a required condition?

Every time.

What is the expected behavior?

The code should not seg fault. It is possible to put the same code in a file (index.js) and run node index.js and it prints true. No seg faults there. Note: you need to put "type": "module" in the package.json to use await at the top level.

$ node index.js 
true

What do you see instead?

A segmentation fault when the code is run inside the REPL.

> await loadGames(10);
Segmentation fault: 11

Additional information

Node was installed on MacOS Monterey version 12.5.1 using nvm https://github.com/nvm-sh/nvm

$ nvm install 16
Downloading and installing node v16.17.0...
Downloading https://nodejs.org/dist/v16.17.0/node-v16.17.0-darwin-x64.tar.xz...
####################################################################################################################################################################################################################################################### 100.0%
Computing checksum with sha256sum
Checksums matched!

Now using node v16.17.0 (npm v8.15.0)
$ nvm use 16
Now using node v16.17.0 (npm v8.15.0)
$ node
Welcome to Node.js v16.17.0.

Found this segfault while trying out this tutorial on using fetch with a custom timeout https://dmitripavlutin.com/timeout-fetch-request/

targos commented 2 years ago

That's probably a duplicate of https://github.com/nodejs/node/issues/38695

HarikrishnanBalagopal commented 2 years ago

That's probably a duplicate of #38695

REPL crashes when doing a dynamic import when the working directory contains a folder with the name src: In my case there is no folder with the name src

martian17 commented 1 year ago

reproduced the same issue with a smaller piece of code

$ node -v
v16.8.0
$ node
Welcome to Node.js v16.8.0.
Type ".help" for more information.
> const fetch = (...args) => import('node-fetch').then(({default: fetch}) => fetch(...args));
undefined
> let res = await fetch("https://www.google.com",{method:"get"});
Segmentation fault: 11
$
martian17 commented 1 year ago

After some experimentation, I found the following workaround that works in all general use cases, including loading it as a file from repl.

const fetch = (()=>{let m = import("node-fetch");return async (...args)=>await (await m).default(...args);})();

Also, if you are manually importing node-fetch in repl, the following code works as well since repl supports await on global scope

> const fetch = (await import('node-fetch')).default;
...do something with fetch
martian17 commented 1 year ago

Here is some explanation of my workaround: Original idiomatic way of importing node-fetch is the following

const fetch = (...args) => import('node-fetch').then(({default: fetch}) => fetch(...args));

Since according to #38695, the problem has to do with premature garbage collection, I thought that if I could manually store aside the result of import(), it could work. And so came my solution. My workaround stores aside the result of import() as promise inside an anonymous function, and access to it using (await mod) whenever the fetch function is called

const fetch = (() => {
    let mod = import("node-fetch");
    return async (...args) => await (await mod).default(...args);
})();

Shortened form, it would look like this

const fetch = (()=>{let m = import("node-fetch");return async (...args)=>await (await m).default(...args);})();