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

napi_create_external_arraybuffer implementation requires upward/circular dependency #49522

Closed
sparist opened this issue Sep 7, 2023 · 5 comments
Labels
node-api Issues and PRs related to the Node-API.

Comments

@sparist
Copy link

sparist commented Sep 7, 2023

Version

18.x-staging head

Platform

No response

Subsystem

Node-API

What steps will reproduce the bug?

Using only the JS portion of the Node API, call napi_create_external_arraybuffer() and note that the ERR_BUFFER_CONTEXT_NOT_AVAILABLE error results.

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

No response

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

The expected behaviour is that functions in the JS-only portion of the Node API can be called without requiring the entire Node environment; this breaks separation of concerns.

What do you see instead?

The following change introduced an upward JS Node API dependency on the Node API:

#32463

For consumers of the JS-only API who don't use the Node portion of the Node API, calling napi_create_external_arraybuffer results in ERR_BUFFER_CONTEXT_NOT_AVAILABLE.

Reverting the change in the above linked issue caused napi_create_external_arraybuffer to again work correctly.

Because the API implementation has deviated significantly since then, it has become impossible to keep my fork up-to-date, even to the tip of 18.x-staging.

Requesting that this be implemented in JS Node API without requiring an upward dependency on Node API, as it was prior to the above change. This decision makes the JS Node API unusable without all of Node, affects my ability to use Node-API (and requires me to use an old fork), and is a generally broken architectural design.

Failing that, [a] workaround[s] that I can apply to keep my fork up-to-date.

Additional information

No response

@bnoordhuis bnoordhuis added the node-api Issues and PRs related to the Node-API. label Sep 7, 2023
@bnoordhuis
Copy link
Member

Using only the JS portion of the Node API, call napi_create_external_arraybuffer() and note that the ERR_BUFFER_CONTEXT_NOT_AVAILABLE error results.

It took me a long time to parse this but I think you're saying you use napi without creating a node runtime, correct?

The expected behaviour is that functions in the JS-only portion of the Node API can be called without requiring the entire Node environment; this breaks separation of concerns.

I don't know if that's a reasonable expectation. I'm not even sure if that's a promise napi makes. I mean, the N stands for Node, not JS.

External arraybuffers need to be tracked (and cleaned up) by something and that's what the CallbackInfo class in src/node_buffer.cc does. napi could maybe1 do that by itself but that results in a lot of duplicated functionality.

I'd say that if you want to see that happen, open a pull request and see how it's received. Maybe you'll find it's simply not technically feasible to break the dependency.

1 or maybe not because of event loop integration

@sparist
Copy link
Author

sparist commented Sep 7, 2023

I think you're saying you use napi without creating a node runtime, correct?

That is correct. Some background: I have a V8 embedding that uses Google Dawn for its WebGPU implementation. Its API is coded for Node-API (and only the JS portion). Therefore, I must use Node-API, but I don't want the higher Node layer that includes the event loop etc.

There is another project, BabylonNative, that maintains a fork of the Node JS API for a similar reason.

I don't know if that's a reasonable expectation. I'm not even sure if that's a promise napi makes. I mean, the N stands for Node, not JS.

Maybe not, but it seems to be bad architectural design to allow the JS portion reach up into Node for just this one thing. Upward dependencies are bad.

External arraybuffers need to be tracked (and cleaned up) by something and that's what the CallbackInfo class in src/node_buffer.cc does. napi could maybe1 do that by itself but that results in a lot of duplicated functionality.

It seems to me that if the JS layer requires this, the implementation should be moved to the JS layer.

I'd say that if you want to see that happen, open a pull request and see how it's received. Maybe you'll find it's simply not technically feasible to break the dependency.

I sadly don't have sufficient time or domain knowledge to code this.

@bnoordhuis
Copy link
Member

I sadly don't have sufficient time or domain knowledge to code this.

Then I suspect it's unlikely to happen. LMK if you change your mind but otherwise please close this.

@sparist
Copy link
Author

sparist commented Sep 7, 2023

I'd prefer to keep it open, since it describes a real architectural defect in the code.

@bnoordhuis
Copy link
Member

That is, with all due deference, just your opinion. For another perspective: it's doing exactly what it's designed to do.

I don't like keeping issues around that aren't going to make progress and I'm not even sure I agree something needs to change so I'll take the liberty of closing it.

@bnoordhuis bnoordhuis closed this as not planned Won't fix, can't repro, duplicate, stale Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

No branches or pull requests

2 participants