buffer,n-api: properly handle Environment cleanup #30551

addaleax · 2019-11-19T20:42:07Z

buffer: release buffers with free callbacks on env exit

Invoke the free callback for a given Buffer if it was created
with one, and mark the underlying ArrayBuffer as detached.

This makes sure that the memory is released e.g. when addons inside
Workers create such Buffers.

n-api: detach external ArrayBuffers on env exit

Make sure that ArrayBuffers created using
napi_create_external_arraybuffer are rendered unusable
after its memory has been released.

test: port worker + buffer test to N-API

This ports test/addons/worker-buffer-callback to N-API,
with the small exception of using external ArrayBuffers rather
than external Node.js Buffers.

Checklist

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

gabrielschulhof

LGTM, but would it be possible to create a subclass of v8impl::Reference for buffers which overrides Finalize and chains up instead of using an extra bool parameter that is buffer-specific?

addaleax · 2019-11-23T12:09:33Z

@gabrielschulhof Done, is this what you had in mind?

nodejs-github-bot · 2019-11-23T12:10:21Z

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

gabrielschulhof

LGTM

test/node-api/test_worker_buffer_callback/test.js

nodejs-github-bot · 2019-11-23T21:37:59Z

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

src/js_native_api_v8.cc

nodejs-github-bot · 2019-11-25T12:18:35Z

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

mhdawson

LGTM

nodejs-github-bot · 2019-11-27T23:10:42Z

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

Invoke the free callback for a given `Buffer` if it was created with one, and mark the underlying `ArrayBuffer` as detached. This makes sure that the memory is released e.g. when addons inside Workers create such `Buffer`s.

Make sure that `ArrayBuffer`s created using `napi_create_external_arraybuffer` are rendered unusable after its memory has been released.

This ports `test/addons/worker-buffer-callback` to N-API, with the small exception of using external `ArrayBuffer`s rather than external Node.js `Buffer`s.

nodejs-github-bot · 2019-11-29T15:31:11Z

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

Invoke the free callback for a given `Buffer` if it was created with one, and mark the underlying `ArrayBuffer` as detached. This makes sure that the memory is released e.g. when addons inside Workers create such `Buffer`s. PR-URL: #30551 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Make sure that `ArrayBuffer`s created using `napi_create_external_arraybuffer` are rendered unusable after its memory has been released. PR-URL: #30551 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

This ports `test/addons/worker-buffer-callback` to N-API, with the small exception of using external `ArrayBuffer`s rather than external Node.js `Buffer`s. PR-URL: #30551 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

addaleax · 2019-11-30T01:03:12Z

Landed in ab2afa3...f4f8ec2

Invoke the free callback for a given `Buffer` if it was created with one, and mark the underlying `ArrayBuffer` as detached. This makes sure that the memory is released e.g. when addons inside Workers create such `Buffer`s. PR-URL: #30551 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Make sure that `ArrayBuffer`s created using `napi_create_external_arraybuffer` are rendered unusable after its memory has been released. PR-URL: #30551 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

This ports `test/addons/worker-buffer-callback` to N-API, with the small exception of using external `ArrayBuffer`s rather than external Node.js `Buffer`s. PR-URL: #30551 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

targos · 2020-01-14T11:00:48Z

Should this be backported to v12.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

targos · 2020-01-14T11:02:38Z

Actually, it looks like part of this PR already landed on v12.x.
The commit that would require a backport is e8af569

Invoke the free callback for a given `Buffer` if it was created with one, and mark the underlying `ArrayBuffer` as detached. This makes sure that the memory is released e.g. when addons inside Workers create such `Buffer`s. PR-URL: nodejs#30551 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Invoke the free callback for a given `Buffer` if it was created with one, and mark the underlying `ArrayBuffer` as detached. This makes sure that the memory is released e.g. when addons inside Workers create such `Buffer`s. PR-URL: #30551 Backport-PR-URL: #31355 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

These calls could fail if the `ArrayBuffer` had already been explicitly detached at some point in the past. The necessary test changes already came with 4f523c2 and could be ported back to v12.x with a backport of this PR. Fixes: nodejs#33022 Refs: nodejs#30551

These calls could fail if the `ArrayBuffer` had already been explicitly detached at some point in the past. The necessary test changes already came with 4f523c2 and could be ported back to v12.x with a backport of this PR. Fixes: #33022 Refs: #30551 PR-URL: #33039 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

addaleax requested a review from gabrielschulhof November 19, 2019 20:42

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Nov 19, 2019

addaleax added node-api Issues and PRs related to the Node-API. worker Issues and PRs related to Worker support. labels Nov 19, 2019

gabrielschulhof approved these changes Nov 23, 2019

View reviewed changes

test/node-api/test_worker_buffer_callback/test.js Outdated Show resolved Hide resolved

lundibundi reviewed Nov 24, 2019

View reviewed changes

src/js_native_api_v8.cc Outdated Show resolved Hide resolved

gabrielschulhof approved these changes Nov 25, 2019

View reviewed changes

cjihrig approved these changes Nov 25, 2019

View reviewed changes

lundibundi approved these changes Nov 26, 2019

View reviewed changes

mhdawson approved these changes Nov 27, 2019

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 27, 2019

addaleax added 3 commits November 29, 2019 16:30

buffer: release buffers with free callbacks on env exit

3051b58

Invoke the free callback for a given `Buffer` if it was created with one, and mark the underlying `ArrayBuffer` as detached. This makes sure that the memory is released e.g. when addons inside Workers create such `Buffer`s.

n-api: detach external ArrayBuffers on env exit

3bd4bbb

Make sure that `ArrayBuffer`s created using `napi_create_external_arraybuffer` are rendered unusable after its memory has been released.

test: port worker + buffer test to N-API

e59af1b

This ports `test/addons/worker-buffer-callback` to N-API, with the small exception of using external `ArrayBuffer`s rather than external Node.js `Buffer`s.

addaleax force-pushed the buffer-cleanup-hooks branch from 18dd053 to e59af1b Compare November 29, 2019 15:30

addaleax closed this Nov 30, 2019

addaleax deleted the buffer-cleanup-hooks branch November 30, 2019 01:03

BethGriggs mentioned this pull request Dec 23, 2019

v12.14.1 proposal #31069

Merged

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

addaleax mentioned this pull request Jan 14, 2020

src: mark ArrayBuffers with free callbacks as untransferable #30475

Closed

4 tasks

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

legendecas mentioned this pull request Apr 24, 2020

napi_detach_arraybuffer aborts in Node 12.16.2 with an external arraybuffer #33022

Closed

addaleax mentioned this pull request Apr 24, 2020

buffer,n-api: fix double ArrayBuffer::Detach() during cleanup #33039

Closed

2 tasks

Uh oh!

buffer,n-api: properly handle Environment cleanup #30551

buffer,n-api: properly handle Environment cleanup #30551

Uh oh!

Conversation

addaleax commented Nov 19, 2019

buffer: release buffers with free callbacks on env exit

n-api: detach external ArrayBuffers on env exit

test: port worker + buffer test to N-API

Checklist

Uh oh!

gabrielschulhof left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Nov 23, 2019

Uh oh!

nodejs-github-bot commented Nov 23, 2019

Uh oh!

gabrielschulhof left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 23, 2019

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 25, 2019

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Nov 27, 2019

Uh oh!

nodejs-github-bot commented Nov 29, 2019

Uh oh!

addaleax commented Nov 30, 2019

Uh oh!

targos commented Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Jan 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

targos commented Jan 14, 2020 •

edited

Loading