X Tutup
The Wayback Machine - https://web.archive.org/web/20250809113832/https://github.com/nodejs/node/pull/59071
Skip to content

node-api: added SharedArrayBuffer api #59071

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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

mertcanaltin
Copy link
Member

@mertcanaltin mertcanaltin commented Jul 14, 2025

added this features in node-api : #23276

node_api_create_external_sharedarraybuffer
node_api_get_sharedarraybuffer_info
node_api_is_sharedarraybuffer

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. labels Jul 14, 2025
@mertcanaltin mertcanaltin force-pushed the mert/added-shared-array-buffer-api branch from 506dc09 to 80236e8 Compare July 14, 2025 15:41
@mertcanaltin mertcanaltin marked this pull request as draft July 14, 2025 15:45
@mertcanaltin
Copy link
Member Author

mertcanaltin commented Jul 14, 2025

I'm wondering whether it makes sense to limit the use of SharedArrayBuffer for security reasons. For example, if I define and restrict kMaxSharedArrayBufferSize, it seems like a good way to improve application security — because if a very large and malicious data is received, it could potentially consume the user's memory.

Copy link

codecov bot commented Jul 14, 2025

Codecov Report

❌ Patch coverage is 70.27027% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.90%. Comparing base (fc4a8af) to head (54c9b03).
⚠️ Report is 139 commits behind head on main.

Files with missing lines Patch % Lines
src/js_native_api_v8.cc 70.27% 0 Missing and 11 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59071      +/-   ##
==========================================
- Coverage   90.09%   89.90%   -0.20%     
==========================================
  Files         645      655      +10     
  Lines      189930   192858    +2928     
  Branches    37217    37808     +591     
==========================================
+ Hits       171125   173385    +2260     
- Misses      11512    12032     +520     
- Partials     7293     7441     +148     
Files with missing lines Coverage Δ
src/js_native_api_v8.cc 76.50% <70.27%> (-0.07%) ⬇️

... and 110 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mertcanaltin mertcanaltin marked this pull request as ready for review July 16, 2025 17:27
@vmoroz
Copy link
Member

vmoroz commented Jul 17, 2025

@mertcanaltin , thank you for adding the new APIs for the shared buffer!
Please follow the same naming conventions, versioning, and other rules as you did for the new Node-API functions that you added before. E.g. using node_api_ prefix, using experimental version, etc.
See: https://github.com/nodejs/node/blob/main/doc/contributing/adding-new-napi-api.md

@mertcanaltin
Copy link
Member Author

@mertcanaltin , thank you for adding the new APIs for the shared buffer! Please follow the same naming conventions, versioning, and other rules as you did for the new Node-API functions that you added before. E.g. using node_api_ prefix, using experimental version, etc. See: https://github.com/nodejs/node/blob/main/doc/contributing/adding-new-napi-api.md

I forgot about this, sorry, updating immediately

@mertcanaltin
Copy link
Member Author

Thank you for Rewiev I have made the arrangements @legendecas

@legendecas legendecas moved this from Need Triage to In Progress in Node-API Team Project Jul 25, 2025
@mertcanaltin mertcanaltin requested a review from legendecas July 26, 2025 15:41
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, thank you for working on this

mertcanaltin and others added 3 commits August 8, 2025 14:03
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
@mertcanaltin
Copy link
Member Author

Overall LGTM, thank you for working on this

You are welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

7 participants
X Tutup