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.

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

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

Closed
wants to merge 3 commits into from

Conversation

@addaleax
Copy link
Member

addaleax commented Nov 19, 2019

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
Copy link
Contributor

gabrielschulhof left a comment

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

This comment has been minimized.

Copy link
Member Author

addaleax commented Nov 23, 2019

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

@nodejs-github-bot

This comment has been minimized.

Copy link
Contributor

gabrielschulhof left a comment

LGTM

test/node-api/test_worker_buffer_callback/test.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

src/js_native_api_v8.cc Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

Copy link
Member

mhdawson left a comment

LGTM

@nodejs-github-bot

This comment has been minimized.

addaleax added 3 commits Nov 19, 2019
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.
@addaleax addaleax force-pushed the addaleax:buffer-cleanup-hooks branch from 18dd053 to e59af1b Nov 29, 2019
@nodejs-github-bot

This comment has been minimized.

addaleax added a commit that referenced this pull request Nov 30, 2019
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>
addaleax added a commit that referenced this pull request Nov 30, 2019
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>
addaleax added a commit that referenced this pull request Nov 30, 2019
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

This comment has been minimized.

Copy link
Member Author

addaleax commented Nov 30, 2019

Landed in ab2afa3...f4f8ec2

@addaleax addaleax closed this Nov 30, 2019
@addaleax addaleax deleted the addaleax:buffer-cleanup-hooks branch Nov 30, 2019
addaleax added a commit that referenced this pull request Nov 30, 2019
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>
addaleax added a commit that referenced this pull request Nov 30, 2019
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>
addaleax added a commit that referenced this pull request Nov 30, 2019
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>
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
targos added a commit that referenced this pull request Dec 5, 2019
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>
targos added a commit that referenced this pull request Dec 5, 2019
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>
@BethGriggs BethGriggs mentioned this pull request Dec 9, 2019
Sebastien-Ahkrin added a commit to Sebastien-Ahkrin/node that referenced this pull request Dec 11, 2019
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>
Sebastien-Ahkrin added a commit to Sebastien-Ahkrin/node that referenced this pull request Dec 11, 2019
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>
Sebastien-Ahkrin added a commit to Sebastien-Ahkrin/node that referenced this pull request Dec 11, 2019
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>
MylesBorins added a commit that referenced this pull request Dec 17, 2019
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>
MylesBorins added a commit that referenced this pull request Dec 17, 2019
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>
@BethGriggs BethGriggs mentioned this pull request Dec 23, 2019
@targos

This comment has been minimized.

Copy link
Member

targos commented Jan 14, 2020

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

This comment has been minimized.

Copy link
Member

targos commented Jan 14, 2020

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

addaleax added a commit to addaleax/node that referenced this pull request Jan 14, 2020
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>
targos added a commit that referenced this pull request Jan 14, 2020
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>
targos added a commit that referenced this pull request Jan 14, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.