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.

Sign up for GitHub

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

Jump to bottom

Electron 21 breaks api compatability with some modules #441

Closed
Julusian opened this issue Sep 24, 2022 · 9 comments
Closed

Electron 21 breaks api compatability with some modules #441

Julusian opened this issue Sep 24, 2022 · 9 comments

Comments

@Julusian
Copy link

Blog post about the change: https://www.electronjs.org/blog/v8-memory-cage, coming to v21, not 20 like the post states.

Essentially calls to napi_create_external_buffer fail with

[827133:0924/184053.372625:ERROR:node_bindings.cc(146)] Fatal error in V8: v8_ArrayBuffer_NewBackingStore When the V8 Sandbox is enabled, ArrayBuffer backing stores must be allocated inside the sandbox address space. Please use an appropriate ArrayBuffer::Allocator to allocate these buffers.

To me this goes directly against the promises of node-api, as 5 of 5 of the native modules I have contributed to will not work in electron 21 without changes.

I don't know if people here have any sway over at electron, but as they currently support the same node-api and will soon be using a subtly different version of node-api (which doesnt have its own documentation), it might be a good idea to talk with them about it
@vladimiry vladimiry mentioned this issue Oct 1, 2022
Closed
@KevinEady KevinEady mentioned this issue Oct 7, 2022
Open
@mhdawson
Copy link
Member

We had some good discussion and talked about what we might be able to do that would be useful.

The only thing we have in mind so far might be to add a #define that can be used to exclude napi_create_external_buffer so that it is easier for package authors to know if there are going to be problems on runtimes that no longer suppoirt it.

We'll think more about it and discuss again next week.

I don't think we have any hope of change electron's mind in terms of the decision they took.

@Julusian
Copy link
Author

Yeah, I think that a define would be a good help here. That will at least help those who are aware enough to enable it.
Hopefully a discussion can be had over whether node-addon-api should enable that define by default in a future release.

I don't think we have any hope of change electron's mind in terms of the decision they took.

Yeah, now that it is in a stable release I dont expect they would.
But hopefully a discussion can be started with them to coordinate changes in the future. As I said in an issue with them, my biggest problem is the lack of notice and documentation. While they did do a blog post which was published a couple of months before, and it was mentioned in the release notes, I expect very few users/maintainers to see those. Any documentation about this api has no mention of this limitation

@mhdawson
Copy link
Member

@Julusian I do agree it would have been nice for some co-ordination with the Node-API team to have been done up front. The more implementers we have the more important that becomes. Where there specific people that you talked to on the electron side who might be good people to reach out to?

@Julusian
Copy link
Author

All the discussion I have had with the elcecton team has been in an issue electron/electron#35801

@Brooooooklyn Brooooooklyn mentioned this issue Oct 17, 2022
Closed
@mhdawson
Copy link
Member

@Julusian thanks for the pointer to the issue/discussion.

@mhdawson
Copy link
Member

This comment from electron/electron#35801

I think I'd still prefer a stable API to detect the cage up-front rather than having the process crash. Failing that, we can add something explicit to the sharp API that allows someone developing for Electron 21+ to opt-in to copying the Buffer (and therefore opt-in to the performance and memory fragmentation cost).

Is worth discussing in the next Node-api team meeting.

@mhdawson mhdawson added this to the Milestone 11 milestone Oct 21, 2022
mhdawson added a commit to mhdawson/io.js that referenced this issue Oct 25, 2022
@mhdawson
node-api: handle no support for external buffers
e29b2c0 
Refs: electron/electron#35801
Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:
- hide the methods to create external buffers so they can
  avoid using them if they want the broadest compatibility.
- call the methods that create external buffers at runtime
  to check if external buffers are supported and either
  use them or not based on the return code.

Signed-off-by: Michael Dawson <[email protected]>
@mhdawson mhdawson mentioned this issue Oct 25, 2022
Closed
@mhdawson
Copy link
Member

Created this PR to do what we discussed in the last node-api team meeting - nodejs/node#45181

mhdawson added a commit to mhdawson/io.js that referenced this issue Nov 1, 2022
@mhdawson
node-api: handle no support for external buffers
bc83ee3 
Refs: electron/electron#35801
Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:
- hide the methods to create external buffers so they can
  avoid using them if they want the broadest compatibility.
- call the methods that create external buffers at runtime
  to check if external buffers are supported and either
  use them or not based on the return code.

Signed-off-by: Michael Dawson <[email protected]>
@NickNaso NickNaso mentioned this issue Nov 2, 2022
Closed
@mhdawson mhdawson removed this from the Milestone 11 milestone Nov 4, 2022
mhdawson added a commit to nodejs/node that referenced this issue Nov 9, 2022
@mhdawson
node-api: handle no support for external buffers
09ae62b 
Refs: electron/electron#35801
Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:
- hide the methods to create external buffers so they can
  avoid using them if they want the broadest compatibility.
- call the methods that create external buffers at runtime
  to check if external buffers are supported and either
  use them or not based on the return code.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #45181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
RafaelGSS pushed a commit to nodejs/node that referenced this issue Nov 10, 2022
@mhdawson @RafaelGSS
node-api: handle no support for external buffers
aaca54c 
Refs: electron/electron#35801
Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:
- hide the methods to create external buffers so they can
  avoid using them if they want the broadest compatibility.
- call the methods that create external buffers at runtime
  to check if external buffers are supported and either
  use them or not based on the return code.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #45181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@KevinEady
Copy link
Contributor

We discussed this in the 11 Nov Node API meeting.

Since nodejs/node#45181 has been merged, there is no additional work for the Node API team regarding this issue. We can close this issue @mhdawson .

@mhdawson
Copy link
Member

@KevinEady at this point we just need the PRs to be backported to earlier release lines, but I'm ok with closing.

richardlau pushed a commit to nodejs/node that referenced this issue Nov 24, 2022
@mhdawson @richardlau
node-api: handle no support for external buffers
5a0efa0 
Refs: electron/electron#35801
Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:
- hide the methods to create external buffers so they can
  avoid using them if they want the broadest compatibility.
- call the methods that create external buffers at runtime
  to check if external buffers are supported and either
  use them or not based on the return code.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #45181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@mhdawson mhdawson mentioned this issue Nov 24, 2022
Closed
richardlau pushed a commit to nodejs/node that referenced this issue Dec 7, 2022
@mhdawson @richardlau
node-api: handle no support for external buffers
2dbeb88 
Refs: electron/electron#35801
Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:
- hide the methods to create external buffers so they can
  avoid using them if they want the broadest compatibility.
- call the methods that create external buffers at runtime
  to check if external buffers are supported and either
  use them or not based on the return code.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #45181
Backport-PR-URL: #45616
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
This was referenced Dec 9, 2022
Merged
Merged
Merged
Merged
@sprocketc sprocketc mentioned this issue Dec 15, 2022
Merged
3 tasks
@Julusian Julusian mentioned this issue Dec 29, 2022
Closed
danielleadams pushed a commit to nodejs/node that referenced this issue Dec 30, 2022
@mhdawson @danielleadams
node-api: handle no support for external buffers
d0b3578 
Refs: electron/electron#35801
Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:
- hide the methods to create external buffers so they can
  avoid using them if they want the broadest compatibility.
- call the methods that create external buffers at runtime
  to check if external buffers are supported and either
  use them or not based on the return code.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #45181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
danielleadams pushed a commit to nodejs/node that referenced this issue Dec 30, 2022
@mhdawson @danielleadams
node-api: handle no support for external buffers
8e89610 
Refs: electron/electron#35801
Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:
- hide the methods to create external buffers so they can
  avoid using them if they want the broadest compatibility.
- call the methods that create external buffers at runtime
  to check if external buffers are supported and either
  use them or not based on the return code.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #45181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
@mhdawson @guangwong
node-api: handle no support for external buffers
97edb07 
Refs: electron/electron#35801
Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:
- hide the methods to create external buffers so they can
  avoid using them if they want the broadest compatibility.
- call the methods that create external buffers at runtime
  to check if external buffers are supported and either
  use them or not based on the return code.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs/node#45181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
@mhdawson @guangwong
node-api: handle no support for external buffers
cd41356 
Refs: electron/electron#35801
Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:
- hide the methods to create external buffers so they can
  avoid using them if they want the broadest compatibility.
- call the methods that create external buffers at runtime
  to check if external buffers are supported and either
  use them or not based on the return code.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs/node#45181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
danielleadams pushed a commit to nodejs/node that referenced this issue Jan 3, 2023
@mhdawson @danielleadams
node-api: handle no support for external buffers
746175a 
Refs: electron/electron#35801
Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:
- hide the methods to create external buffers so they can
  avoid using them if they want the broadest compatibility.
- call the methods that create external buffers at runtime
  to check if external buffers are supported and either
  use them or not based on the return code.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #45181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
danielleadams pushed a commit to nodejs/node that referenced this issue Jan 4, 2023
@mhdawson @danielleadams
node-api: handle no support for external buffers
bf8466a 
Refs: electron/electron#35801
Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:
- hide the methods to create external buffers so they can
  avoid using them if they want the broadest compatibility.
- call the methods that create external buffers at runtime
  to check if external buffers are supported and either
  use them or not based on the return code.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #45181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@AlCalzone AlCalzone mentioned this issue Jan 26, 2023
Closed
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
@mhdawson
node-api: handle no support for external buffers
bc62153 
Refs: electron/electron#35801
Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:
- hide the methods to create external buffers so they can
  avoid using them if they want the broadest compatibility.
- call the methods that create external buffers at runtime
  to check if external buffers are supported and either
  use them or not based on the return code.

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs/node#45181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Julusian @KevinEady @mhdawson