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.

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

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

Open
HarikrishnanBalagopal opened this issue Aug 31, 2022 · 5 comments

Comments

@HarikrishnanBalagopal
Copy link

HarikrishnanBalagopal commented Aug 31, 2022

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/

@HarikrishnanBalagopal HarikrishnanBalagopal changed the title segmentation fault in REPL when trying to use AbortController with node-fetch package, dynamic import and timeouts bug: segmentation fault in REPL when trying to use AbortController with node-fetch package, dynamic import and timeouts Aug 31, 2022
@targos
Copy link
Member

targos commented Aug 31, 2022

That's probably a duplicate of #38695

@HarikrishnanBalagopal
Copy link
Author

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
Copy link

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
Copy link

martian17 commented Sep 30, 2022

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
Copy link

martian17 commented Sep 30, 2022

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);})();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants