Use only one copy of llhttp ? #44000

kapouer · 2022-07-26T15:14:27Z

What is the problem this feature will solve?

Versions of Node using Undici (16, 18) are actually using two different copies of llhttp

What is the feature you are proposing to solve the problem?

Would it make sense to use llhttp wasm that is bundled in Undici, instead of recompiling it ?

What alternatives have you considered?

n/a it's a refactoring.

aduh95 · 2022-07-26T17:14:48Z

Or could the bundled Undici use the native one? @nodejs/undici

mcollina · 2022-07-26T21:36:29Z

The architecture and integration of the two is radically different.

When switching to the wasm one we actually got an improvement in performance.

I think it might be possible to reach a stage where this is possible, but it's probably far from now given we are focusing on fetch. However, things change if people would like to work on this.

kasicka · 2022-08-02T14:53:35Z

@mcollina This is a problem for us as distro package maintainers as well. Packaging precompiled code is a no-no for us. Is there a way this could be compiled from source during build? How complicated would it be to use non-wasm version given we are okay with worse performance? Or would it be plausible to have an option to have node without undici? IIRC it's used only for fetch which is experimental for now.

@khardix @mhdawson @sgallagher

kapouer · 2022-08-02T14:58:39Z

@kasicka you should have a look at how debian is dealing with it.
node-undici package is building the wasm files from source, there, and nodejs package embeds the result.

mcollina · 2022-08-02T16:12:35Z

@nodejs/tsc we have a decision to make here. One of the reasons we stopped using the native http_parser was due to the use of process.bindings() for retrieving it (nodejs/undici#22 (comment))
Assuming we don't want to bring process.binding() usage back, we'd need to create some custom wrapping code for undici that replaces that part. Are we all onboard?

The second question is who is willing to do the work. Are there any volunteers?

mhdawson · 2022-08-02T19:58:06Z

I have been thinking about how we might move towards a standardized approach for integrating WASM which would address some of the concerns on the distro side. Some aspects of that would include:

Making it easy to identify/find all WASM blobs
Having good process/es/documentation for how the WASM blobs are generated in a repeatable way. I think this is also good for the project ignoring the distros in terms of supply chain security.
Making it easy for a downstream project to follow 2) and then replace the blogs from 1) with versions that they regenerate.

@kapouer when you say the nodejs package embeds the result, is it replacing what is in the Node.js upstream by default or some other process?

mcollina · 2022-08-02T20:52:29Z

FWIW building the wasm needed by undici is completely automated (otherwise is very hard to do it by hand).

kapouer · 2022-08-02T21:03:37Z

@mhdawson currently the nodejs debian package replaces deps/undici/undici.js during build by the one built by the node-undici debian package using a slightly patched version of undici's build/wasm.js.
However, that's suboptimal for us, since rebuilding nodejs each time undici is updated in debian is not a good idea (especially for security fixes). It is not a big problem though, as it's rather easy to patch nodejs so that it requires the external undici-fetch.js file instead of bundling it.

mhdawson · 2022-08-03T19:46:43Z

@kapouer thanks for the details.

mhdawson · 2022-08-03T20:20:41Z

as it's rather easy to patch nodejs so that it requires the external undici-fetch.js file instead of bundling it.

I'm thinking that as there are more instances like this, it may become more of an overhead. I assume having a configure time option would make it a bit easier.

Thinking out loud a standard approach might be something like:

Any WASM/generated JavaScript that is not created directly as part of the Node.js build should be retrieved through a common accessor method.
For community builds the WASM/generated JavaScript that is required will be bundled into Node.js. We could probably come up with some infra where adding another instance would be as easy as, point to file in deps, give it a name, its bundled in and accessible through the name given using the accessor method.
In each case for 1) there should be a configure time option which allows the bundled require to be replaced
with a require for an external file and for the path(s) for where to look for those files. The configuration time options might even be able to be autogenerated based on the name used in 2)

If the same approach was used in undichi (or at least the same configuration option name) for example, then the option could be simply passed on and the same external file could be used by Node.js core and undichi (although we would still have 2 copies)

I'm thinking of this kind of the along the same lines as how we have configure time options to allow components like openssl, cares, etc. to either be bundled in to the node.js binary or linked against as a shared library.

We might even be able to use the same approach as for shared libraries by getting the WASM/generated JavaScript from a symbol that is either present in the binary itself or within a shared library but that does seem a bit like putting a square peg in a round hole.

GeoffreyBooth · 2022-08-03T21:44:29Z

Why is it necessary to use external versions? Could Debian’s patch be integrated into Uncici’s version somehow?

We include a wasm binary for the https://github.com/nodejs/cjs-module-lexer dependency that’s also bundled in Node. I thought that this binary was good to run anywhere without needing recompilation. It feels like this is the model we should be shooting for, that Node’s binaries/scripts can run anywhere, rather than allowing more ways to run custom versions.

kapouer · 2022-08-03T22:00:15Z

@GeoffreyBooth there two use cases (that ideally are selected by ./configure flags, or worse, by local patches):

Node authors distribute a binary bundling many other libraries as much as possible,
Distributions redistribute Node linked with shared (system-installed) libraries as much as possible.

Whatever the module is (cjs-module-lexer, undici, acorn, openssl, c-ares, uv, etc...) if it needs to be compiled to be used,
then it can be treated like a library with respect to those two cases.

In any case, open-source distributions need to recompile each part at some point, during Node build, or during the externalized dependency build.
Also case 2 hates bundling an external dependency that is built and system-installed - security fixes then require a rebuild that could have been avoided by loading dynamically (like shared libraries do).

khardix · 2022-08-04T09:43:51Z

It feels like this is the model we should be shooting for, that Node’s binaries/scripts can run anywhere, rather than allowing more ways to run custom versions.

That is commendable, as long as you trust the person making the binaries. In Fedora (and derivatives), there is a strict policy of not shipping pre-built anything. One of the reasons why we do this is that there is no guarantee that the compiled binary corresponds to the source in git, and does not include i.e. added crypto-miner or other kind of supply chain attack. Rather that take that risk, we want to rebuild everything from source that we can review, using tools we also built ourselves (so no externally prepared SDKs or similar shortcuts either).

That's at least the theory; in practice, corners are sometimes cut and exceptions granted, but we would very much have as little of these as possible.

kapouer · 2022-08-04T09:49:16Z

I second @khardix : it's "open-source distribution" as in: everything is built from open source code.

mhdawson · 2022-08-04T20:48:14Z

@kapouer, @khardix, @kasicka as consumers of what I've proposed in #44000 (comment) what are your thoughts?

kapouer · 2022-08-05T06:56:46Z

Perfect !

mhdawson · 2022-08-17T14:55:04Z

Confirmed with @kasicka and @khardix that the proposal above is reasonable from their perspective as well. Will look at experiementing to see how it might be implemented.

Refs: nodejs#44000 - add infra to support externally shared js builtins in support of distos that want to externalize deps that include JS/WASM instead of native code - add support for externalizing - cjs_module_lexer/lexer - cjs_module_lexer/dist/lexer - undici/undici Signed-off-by: Michael Dawson <[email protected]>

Refs: #44000 - add infra to support externally shared js builtins in support of distos that want to externalize deps that include JS/WASM instead of native code - add support for externalizing - cjs_module_lexer/lexer - cjs_module_lexer/dist/lexer - undici/undici Signed-off-by: Michael Dawson <[email protected]> PR-URL: #44376 Reviewed-By: Gireesh Punathil <[email protected]>

Refs: nodejs#44000 - add infra to support externally shared js builtins in support of distos that want to externalize deps that include JS/WASM instead of native code - add support for externalizing - cjs_module_lexer/lexer - cjs_module_lexer/dist/lexer - undici/undici Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#44376 Reviewed-By: Gireesh Punathil <[email protected]>

Refs: #44000 - add infra to support externally shared js builtins in support of distos that want to externalize deps that include JS/WASM instead of native code - add support for externalizing - cjs_module_lexer/lexer - cjs_module_lexer/dist/lexer - undici/undici Signed-off-by: Michael Dawson <[email protected]> PR-URL: #44376 Reviewed-By: Gireesh Punathil <[email protected]>

nodejs/node#44000

* chore: bump node in DEPS to v18.13.0 * child_process: validate arguments for null bytes nodejs/node#44782 * bootstrap: merge main thread and worker thread initializations nodejs/node#44869 * module: ensure relative requires work from deleted directories nodejs/node#42384 * src: add support for externally shared js builtins nodejs/node#44000 * lib: disambiguate `native module` to `binding` nodejs/node#45673 * test: convert test-debugger-pid to async/await nodejs/node#45179 * deps: upgrade to libuv 1.44.2 nodejs/node#42340 * src: fix cppgc incompatibility in v8 nodejs/node#43521 * src: use qualified `std::move` call in node_http2 nodejs/node#45555 * build: fix env.h for cpp20 nodejs/node#45516 * test: remove experimental-wasm-threads flag nodejs/node#45074 * src: iwyu in cleanup_queue.cc nodejs/node#44983 * src: add missing include for `std::all_of` nodejs/node#45541 * deps: update ICU to 72.1 nodejs/node#45068 * chore: fixup patch indices * chore: remove errant semicolons - nodejs/node#44179 - nodejs/node#44193 * src: add support for externally shared js builtins nodejs/node#44376 * chore: add missing GN filenames * deps: update nghttp2 to 1.51.0 nodejs/node#45537 * chore: disable more Node.js snapshot tests The Snapshot feature is currently disabled * chore: disable ICU timezone tests Node.js uses a different version of ICU than Electron so they will often be out of sync. * chore: disable threadpool event tracing test Event tracing is not enabled in embedded Node.js * chore: fixup patch indices * chore: comments from review Co-authored-by: electron-roller[bot] <84116207 electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <[email protected]>

github-actions · 2023-02-14T01:28:55Z

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

* chore: bump node in DEPS to v18.13.0 * child_process: validate arguments for null bytes nodejs/node#44782 * bootstrap: merge main thread and worker thread initializations nodejs/node#44869 * module: ensure relative requires work from deleted directories nodejs/node#42384 * src: add support for externally shared js builtins nodejs/node#44000 * lib: disambiguate `native module` to `binding` nodejs/node#45673 * test: convert test-debugger-pid to async/await nodejs/node#45179 * deps: upgrade to libuv 1.44.2 nodejs/node#42340 * src: fix cppgc incompatibility in v8 nodejs/node#43521 * src: use qualified `std::move` call in node_http2 nodejs/node#45555 * build: fix env.h for cpp20 nodejs/node#45516 * test: remove experimental-wasm-threads flag nodejs/node#45074 * src: iwyu in cleanup_queue.cc nodejs/node#44983 * src: add missing include for `std::all_of` nodejs/node#45541 * deps: update ICU to 72.1 nodejs/node#45068 * chore: fixup patch indices * chore: remove errant semicolons - nodejs/node#44179 - nodejs/node#44193 * src: add support for externally shared js builtins nodejs/node#44376 * chore: add missing GN filenames * deps: update nghttp2 to 1.51.0 nodejs/node#45537 * chore: disable more Node.js snapshot tests The Snapshot feature is currently disabled * chore: disable ICU timezone tests Node.js uses a different version of ICU than Electron so they will often be out of sync. * chore: disable threadpool event tracing test Event tracing is not enabled in embedded Node.js * chore: fixup patch indices * chore: comments from review Co-authored-by: electron-roller[bot] <84116207 electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <[email protected]>

Refs: #44000 - add infra to support externally shared js builtins in support of distos that want to externalize deps that include JS/WASM instead of native code - add support for externalizing - cjs_module_lexer/lexer - cjs_module_lexer/dist/lexer - undici/undici Signed-off-by: Michael Dawson <[email protected]> PR-URL: #44376 Backport-PR-URL: #45867 Reviewed-By: Gireesh Punathil <[email protected]>

github-actions · 2023-08-14T01:21:20Z

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

github-actions · 2023-09-13T01:22:45Z

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

kapouer added the feature request Issues that request new features to be added to Node.js. label Jul 26, 2022

aduh95 added build Issues and PRs related to build files or the CI. fetch Issues and PRs related to the Fetch API labels Jul 26, 2022

mhdawson mentioned this issue Aug 24, 2022

src: add support for externally shared js builtins #44376

Closed

targos added this to Node.js feature requests Oct 22, 2022

targos moved this to Pending Triage in Node.js feature requests Oct 22, 2022

mhdawson mentioned this issue Dec 14, 2022

[v16.x] src: add support for externally shared js builtins #45867

Closed

codebytere added a commit to electron/electron that referenced this issue Jan 9, 2023

src: add support for externally shared js builtins

0803592

nodejs/node#44000

github-actions bot added the stale label Feb 14, 2023

mhdawson removed the stale label Feb 14, 2023

github-actions bot added the stale label Aug 14, 2023

github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 13, 2023

richardlau mentioned this issue Nov 13, 2023

build: use biome as formatter #50672

Closed

3 tasks

RedYetiDev removed this from Node.js feature requests Jun 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use only one copy of llhttp ? #44000

Use only one copy of llhttp ? #44000

kapouer commented Jul 26, 2022

aduh95 commented Jul 26, 2022

mcollina commented Jul 26, 2022

kasicka commented Aug 2, 2022

kapouer commented Aug 2, 2022

mcollina commented Aug 2, 2022

mhdawson commented Aug 2, 2022

mcollina commented Aug 2, 2022

kapouer commented Aug 2, 2022

mhdawson commented Aug 3, 2022

mhdawson commented Aug 3, 2022 •

edited

Loading

GeoffreyBooth commented Aug 3, 2022

kapouer commented Aug 3, 2022

khardix commented Aug 4, 2022

kapouer commented Aug 4, 2022

mhdawson commented Aug 4, 2022

kapouer commented Aug 5, 2022

mhdawson commented Aug 17, 2022

github-actions bot commented Feb 14, 2023

github-actions bot commented Aug 14, 2023

github-actions bot commented Sep 13, 2023

Use only one copy of llhttp ? #44000

Use only one copy of llhttp ? #44000

Comments

kapouer commented Jul 26, 2022

What is the problem this feature will solve?

What is the feature you are proposing to solve the problem?

What alternatives have you considered?

aduh95 commented Jul 26, 2022

mcollina commented Jul 26, 2022

kasicka commented Aug 2, 2022

kapouer commented Aug 2, 2022

mcollina commented Aug 2, 2022

mhdawson commented Aug 2, 2022

mcollina commented Aug 2, 2022

kapouer commented Aug 2, 2022

mhdawson commented Aug 3, 2022

mhdawson commented Aug 3, 2022 • edited Loading

GeoffreyBooth commented Aug 3, 2022

kapouer commented Aug 3, 2022

khardix commented Aug 4, 2022

kapouer commented Aug 4, 2022

mhdawson commented Aug 4, 2022

kapouer commented Aug 5, 2022

mhdawson commented Aug 17, 2022

github-actions bot commented Feb 14, 2023

github-actions bot commented Aug 14, 2023

github-actions bot commented Sep 13, 2023

mhdawson commented Aug 3, 2022 •

edited

Loading