src: mark ArrayBuffers with free callbacks as untransferable #30475

addaleax · 2019-11-13T19:41:00Z

More precisely, make them untransferable if they were created through
our APIs, because those do not follow the improved free callback
mechanism that V8 uses now. All other ArrayBuffers can be transferred
between threads now, the assumption being that they were created in a
clean way that follows the V8 API on this.

This addresses a TODO comment.

Refs: #30339 (comment)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

More precisely, make them untransferable if they were created through *our* APIs, because those do not follow the improved free callback mechanism that V8 uses now. All other ArrayBuffers can be transferred between threads now, the assumption being that they were created in a clean way that follows the V8 API on this. This addresses a TODO comment. Refs: nodejs#30339 (comment)

devnexen · 2019-11-15T15:21:40Z

src/js_native_api_v8.h

@@ -39,6 +39,10 @@ struct napi_env__ {
  inline void Unref() { if ( --refs == 0) delete this; }

  virtual bool can_call_into_js() const { return true; }
+  virtual v8::Maybe<bool> mark_arraybuffer_as_untransferable(
+      v8::Local<v8::ArrayBuffer> ab) const {


nit: unused ?

@devnexen This method is called in js_native_api_v8.cc (right above this file in the github diff)

In fact I mentioned the argument :-) sorry for the confusion.

Yeah, the argument is not used here, only in the subclass override

nodejs-github-bot · 2019-11-16T16:57:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/26633/

lundibundi · 2019-11-17T12:29:49Z

Well, we've got an ICE =):
Not sure how to report this considering the amount of code involved.

https://github.com/nodejs/node/commit/44da3b3986f9bb5eb055150456ad99452bd010b5
debian9-docker-armv7

https://ci.nodejs.org/job/node-test-commit-arm/27364/nodes=debian9-docker-armv7/console
https://ci.nodejs.org/job/node-test-commit-arm/nodes=debian9-docker-armv7/27364/timestamps/?time=HH:mm:ss&timeZone=GMT+2&appendLog&locale=en_US


21:56:50 In file included from ../src/env.cc:8:0:
21:56:50 ../src/node_file.h: In member function 'void node::fs::FSReqPromise<AliasedBufferT>::Reject(v8::Local<v8::Value>) [with AliasedBufferT = node::AliasedBufferBase<long long unsigned int, v8::BigUint64Array>]':
21:56:50 ../src/node_file.h:238:69: internal compiler error: Segmentation fault
21:56:50      Local<Promise::Resolver> resolver = value.As<Promise::Resolver>();
21:56:50                                                                      ^
21:56:50 Please submit a full bug report,
21:56:50 with preprocessed source if appropriate.
21:56:50 See <file:///usr/share/doc/gcc-6/README.Bugs> for instructions.

The other error is unused port2 in the test.

addaleax · 2019-11-18T12:22:08Z

@lundibundi Thanks for pointing that out! I’ve fixed the linting error and will re-start CI.

As for the ICE, I’m not sure if it helps, but I’ll open a PR to do what should have been done anyway, namely move the code of the method that’s causing the crash into the .cc file.

nodejs-github-bot · 2019-11-18T12:22:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/26696/

- Move inline functions into an `-inl.h` file - Move override function definitions into `.cc` files - Remove `using` statements from header files - Make data fields of classes private - Mark classes at the end of hierarchies as `final` This is also partially being done in an attempt to avoid a particular internal compiler error, see nodejs#30475 (comment) for details.

nodejs-github-bot · 2019-11-18T16:47:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/26710/

More precisely, make them untransferable if they were created through *our* APIs, because those do not follow the improved free callback mechanism that V8 uses now. All other ArrayBuffers can be transferred between threads now, the assumption being that they were created in a clean way that follows the V8 API on this. This addresses a TODO comment. Refs: #30339 (comment) PR-URL: #30475 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

addaleax · 2019-11-19T13:02:00Z

Landed in 6cb8e4b

More precisely, make them untransferable if they were created through *our* APIs, because those do not follow the improved free callback mechanism that V8 uses now. All other ArrayBuffers can be transferred between threads now, the assumption being that they were created in a clean way that follows the V8 API on this. This addresses a TODO comment. Refs: #30339 (comment) PR-URL: #30475 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

- Move inline functions into an `-inl.h` file - Move override function definitions into `.cc` files - Remove `using` statements from header files - Make data fields of classes private - Mark classes at the end of hierarchies as `final` This is also partially being done in an attempt to avoid a particular internal compiler error, see #30475 (comment) for details. PR-URL: #30530 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

targos · 2020-01-14T13:22:52Z

This needs a backport or other previous PRs to be backported in order to land on v12.x-staging.

addaleax · 2020-01-14T13:25:33Z

@targos Yeah, removed the dont-land-on labels because I’m backporting this together with the remaining commit in #30551 – already working on it :)

More precisely, make them untransferable if they were created through *our* APIs, because those do not follow the improved free callback mechanism that V8 uses now. All other ArrayBuffers can be transferred between threads now, the assumption being that they were created in a clean way that follows the V8 API on this. This addresses a TODO comment. Refs: nodejs#30339 (comment) PR-URL: nodejs#30475 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

More precisely, make them untransferable if they were created through *our* APIs, because those do not follow the improved free callback mechanism that V8 uses now. All other ArrayBuffers can be transferred between threads now, the assumption being that they were created in a clean way that follows the V8 API on this. This addresses a TODO comment. Refs: #30339 (comment) PR-URL: #30475 Backport-PR-URL: #31355 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

- Move inline functions into an `-inl.h` file - Move override function definitions into `.cc` files - Remove `using` statements from header files - Make data fields of classes private - Mark classes at the end of hierarchies as `final` This is also partially being done in an attempt to avoid a particular internal compiler error, see #30475 (comment) for details. PR-URL: #30530 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

More precisely, make them untransferable if they were created through *our* APIs, because those do not follow the improved free callback mechanism that V8 uses now. All other ArrayBuffers can be transferred between threads now, the assumption being that they were created in a clean way that follows the V8 API on this. This addresses a TODO comment. Refs: #30339 (comment) PR-URL: #30475 Backport-PR-URL: #31355 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

addaleax added buffer Issues and PRs related to the buffer subsystem. addons Issues and PRs related to native addons. node-api Issues and PRs related to the Node-API. dont-land-on-v10.x labels Nov 13, 2019

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 13, 2019

cjihrig approved these changes Nov 13, 2019

View reviewed changes

jasnell approved these changes Nov 14, 2019

View reviewed changes

devnexen approved these changes Nov 15, 2019

View reviewed changes

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 16, 2019

fixup! src: mark ArrayBuffers with free callbacks as untransferable

47ff8a9

addaleax mentioned this pull request Nov 18, 2019

src: clean up node_file.h #30530

Closed

2 tasks

addaleax closed this Nov 19, 2019

addaleax deleted the buffer-untransferable branch November 19, 2019 13:02

BridgeAR mentioned this pull request Nov 21, 2019

v13.2.0 proposal #30547

Merged

addaleax removed dont-land-on-v10.x labels Jan 14, 2020

targos added the backport-requested-v12.x label Jan 14, 2020

addaleax mentioned this pull request Jan 14, 2020

[v12.x] backport Buffer cleanup changes #31355

Closed

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

targos added backported-to-v12.x and removed backport-requested-v12.x labels Feb 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: mark ArrayBuffers with free callbacks as untransferable #30475

src: mark ArrayBuffers with free callbacks as untransferable #30475

addaleax commented Nov 13, 2019

devnexen Nov 15, 2019

addaleax Nov 16, 2019

devnexen Nov 16, 2019 •

edited

Loading

addaleax Nov 16, 2019

nodejs-github-bot commented Nov 16, 2019

lundibundi commented Nov 17, 2019

addaleax commented Nov 18, 2019

nodejs-github-bot commented Nov 18, 2019

nodejs-github-bot commented Nov 18, 2019

addaleax commented Nov 19, 2019

targos commented Jan 14, 2020

addaleax commented Jan 14, 2020

src: mark ArrayBuffers with free callbacks as untransferable #30475

src: mark ArrayBuffers with free callbacks as untransferable #30475

Conversation

addaleax commented Nov 13, 2019

Checklist

devnexen Nov 15, 2019

Choose a reason for hiding this comment

addaleax Nov 16, 2019

Choose a reason for hiding this comment

devnexen Nov 16, 2019 • edited Loading

Choose a reason for hiding this comment

addaleax Nov 16, 2019

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 16, 2019

lundibundi commented Nov 17, 2019

addaleax commented Nov 18, 2019

nodejs-github-bot commented Nov 18, 2019

nodejs-github-bot commented Nov 18, 2019

addaleax commented Nov 19, 2019

targos commented Jan 14, 2020

addaleax commented Jan 14, 2020

devnexen Nov 16, 2019 •

edited

Loading