nodejs / node

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

C++ Embedding - HTTP Module + fetch() Doesn't Work Outside node::LoadEnvironment #48760

Open Jamie0 opened 1 year ago

Jamie0 commented 1 year ago

Version

18.16.1

Platform

Darwin 19.6.0 Darwin Kernel Version 19.6.0: Mon Aug 31 22:12:52 PDT 2020; root:xnu-6153.141.2~1/RELEASE_X86_64 x86_64

Subsystem

embedding

What steps will reproduce the bug?

  1. Define a function that includes some use of network API (e.g. fetch or net.Socket.prototype.connect)
  2. In C++, invoke this function e.g. through fn->Call or value->CallAsFunction
  3. Attempt to spin the event loop ( e.g. uv_run(loop, UV_RUN_DEFAULT) ) until uv_loop_alive(loop) == 0 to open the socket

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

I can reproduce this reliably (see additional information). The I/O operation has to involve network - e.g. await fs.promises.readFile('test.txt') works correctly

What is the expected behavior? Why is that the expected behavior?

The network request should succeed.

What do you see instead?

uv_loop_alive(loop) immediately returns 0, and any subsequent call to uv_loop block and never return.

Additional information

I've tried many different ways of running the event loop and the result is the same - if the network request was started from Call or CallAsFunction, the work never reaches the event loop. If it is scheduled inside node::LoadEnvironment, the request succeeds.

Although I am not familiar with the nodejs internals, I have been working on the assumption something inside node is not scheduling the network request before Call[AsFunction] returns and execution of the context stops. Following the v8 documentation, I've tried executing PerformMicrotaskCheckpoint, FlushForegroundTasks, and DrainTasks before spinning the loop to no avail.

For context, I am attempting to build a nginx module to extend the nginx web server with node modules. Code that reproduces the bug is on the project's repo - https://github.com/Jamie0/nginx-nodejs-module

I also note if testing a net.Socket(), the 'close' callback immediately executes if attempting to use a DNS name rather than an IP address. With fetch, the behaviour is the same regardless of host name.

If this is intended behaviour when using Call[AsFunction], I would be grateful if the documentation could be updated to explain how one can achieve this.

bnoordhuis commented 1 year ago

LoadEnvironment() initializes a bunch of libuv handles that node uses to drive immediates, timers, microtasks, etc. so that's why you need it.

It'd be possible to pull apart LoadEnvironment() into separate initialization and "run script" steps although passing an empty script to LoadEnvironment() and calling node::MakeCallback() probably works too.

I don't know exactly how you'd document that but pull request welcome. Cool project, by the way.

Jamie0 commented 1 year ago

Ah yes! I am using node::LoadEnvironment to bootstrap the isolate and store a function to run later, but I hadn't come across the node::MakeCallback method as a way of running that function.

Substituting the call ->Call[AsFunction] with that method was enough to make net.Socket() work! Unfortunately the built-in http module and globalThis.fetch() still do not initiate a connection and stall - however I think their UV handles are now correctly registered as the event loop now stays alive indefinitely.

(Thank you for the kind words! It's my first major C++ project (and indeed my first time embedding node/working with V8 APIs). I'd love to contribute to the embedding documentation with some hints from what I've learned - once I find some time I shall open a pull request 😊)

bnoordhuis commented 1 year ago

There's probably additional machinery that you miss out if you only call uv_run(). I'm thinking of CommonEnvironmentSetup and node::SpinEventLoop() but unfortunately the former isn't exposed and the latter blocks.

Apropos nginx, I wrote something like what you're working on years ago (but with plain V8 instead of node) but I eventually moved V8 out of the nginx workers and into separate processes.

The problem I couldn't work around is that V8 does not deal with out-of-memory conditions gracefully. It simply aborts and that didn't fail just the current request but all requests and connections that were being handled by that worker.

Node has the same issue as V8, only more so.