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/


        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 `Buffer`s.


        n-api: detach external ArrayBuffers on env exit

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

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/


        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 `Buffer`s. PR-URL: #30551 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>


        n-api: detach external ArrayBuffers on env exit

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 <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>


        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 `ArrayBuffer`s rather than external Node.js `Buffer`s. PR-URL: #30551 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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

Landed in ab2afa3...f4f8ec2


        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 `Buffer`s. PR-URL: #30551 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>


        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 `ArrayBuffer`s rather than external Node.js `Buffer`s. PR-URL: #30551 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>


        n-api: detach external ArrayBuffers on env exit

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 <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>


        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 `ArrayBuffer`s rather than external Node.js `Buffer`s. PR-URL: #30551 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>


        n-api: detach external ArrayBuffers on env exit

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 <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>


        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 `ArrayBuffer`s rather than external Node.js `Buffer`s. PR-URL: #30551 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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


        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 `Buffer`s. PR-URL: nodejs#30551 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>


        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 `Buffer`s. PR-URL: #30551 Backport-PR-URL: #31355 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>


        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 `Buffer`s. PR-URL: #30551 Backport-PR-URL: #31355 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>


        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 `Buffer`s. PR-URL: #30551 Backport-PR-URL: #31355 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>


        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 `Buffer`s. PR-URL: nodejs#30551 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>


        n-api: detach external ArrayBuffers on env exit

Make sure that `ArrayBuffer`s created using `napi_create_external_arraybuffer` are rendered unusable after its memory has been released. PR-URL: nodejs#30551 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>


        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 `ArrayBuffer`s rather than external Node.js `Buffer`s. PR-URL: nodejs#30551 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>


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

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

addaleax requested a review from gabrielschulhof Nov 19, 2019

nodejs-github-bot added buffer C++ labels Nov 19, 2019

addaleax added n-api worker labels Nov 19, 2019

gabrielschulhof approved these changes Nov 23, 2019

View changes

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

lundibundi reviewed Nov 24, 2019

View changes

src/js_native_api_v8.cc Outdated Show resolved Hide resolved

gabrielschulhof approved these changes Nov 25, 2019

View changes

cjihrig approved these changes Nov 25, 2019

View changes

lundibundi approved these changes Nov 26, 2019

View changes

mhdawson approved these changes Nov 27, 2019

View changes

addaleax added the author ready label Nov 27, 2019

addaleax added 3 commits Nov 19, 2019

addaleax force-pushed the addaleax:buffer-cleanup-hooks branch to e59af1b Nov 29, 2019

addaleax closed this Nov 30, 2019

addaleax deleted the addaleax:buffer-cleanup-hooks branch Nov 30, 2019

BridgeAR mentioned this pull request Dec 3, 2019

v13.3.0 proposal #30774

Merged

BethGriggs mentioned this pull request Dec 9, 2019

v12.13.2 proposal #30865

Closed

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 of 4 tasks complete

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

Open

addaleax mentioned this pull request Apr 24, 2020

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

Open

2 of 2 tasks complete

nodejs / node

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

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

addaleax commented Nov 19, 2019

gabrielschulhof left a comment

This comment has been minimized.

addaleax commented Nov 23, 2019

This comment has been minimized.

nodejs-github-bot commented Nov 23, 2019

gabrielschulhof left a comment

This comment has been minimized.

nodejs-github-bot commented Nov 23, 2019

This comment has been minimized.

nodejs-github-bot commented Nov 25, 2019

mhdawson left a comment

This comment has been minimized.

nodejs-github-bot commented Nov 27, 2019

This comment has been minimized.

nodejs-github-bot commented Nov 29, 2019

This comment has been minimized.

addaleax commented Nov 30, 2019

This comment has been minimized.

targos commented Jan 14, 2020 •

edited

This comment has been minimized.

targos commented Jan 14, 2020

nodejs / node

Join GitHub today

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

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

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

gabrielschulhof left a comment

This comment has been minimized.

addaleax commented Nov 23, 2019

This comment has been minimized.

nodejs-github-bot commented Nov 23, 2019

gabrielschulhof left a comment

This comment has been minimized.

nodejs-github-bot commented Nov 23, 2019

This comment has been minimized.

nodejs-github-bot commented Nov 25, 2019

mhdawson left a comment

This comment has been minimized.

nodejs-github-bot commented Nov 27, 2019

This comment has been minimized.

nodejs-github-bot commented Nov 29, 2019

This comment has been minimized.

addaleax commented Nov 30, 2019

This comment has been minimized.

targos commented Jan 14, 2020 • edited

This comment has been minimized.

targos commented Jan 14, 2020

targos commented Jan 14, 2020 •

edited