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.

Sign up for GitHub

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

Jump to bottom

async_wrap: fix memory leak in AsyncResource #20668

Closed
wants to merge 1 commit into from
Closed

async_wrap: fix memory leak in AsyncResource #20668

wants to merge 1 commit into from

Conversation

mhdawson
Copy link
Member

Reset the persistent that keeps the resource
Object alive when the AsyncResource is being
destroyed.

Fixes: nodejs/node-addon-api#237

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@mhdawson
async_wrap: fix memory leak in AsyncResource
2016060 
Reset the persistent that keeps the resource
Object alive when the AsyncResource is being
destroyed.

Fixes: nodejs/node-addon-api#237
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 11, 2018
@mhdawson mhdawson mentioned this pull request May 11, 2018
Closed
@addaleax
Copy link
Member

We might also want an option to toggle a weak/non-weak flag, like we have for references

TimothyGu
TimothyGu approved these changes May 11, 2018
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

I was going to ask if we could use node::Persistent instead, but then I realized that was an internal API. LGTM.

@bnoordhuis
Copy link
Member

bnoordhuis commented May 11, 2018 •
edited
Loading

Regrettably that's not going to fix the memory leak in already compiled add-ons since it's an inline definition. Can I suggest moving the method bodies to a .cc file?

edit: to be clear, that won't fix the add-on issue right away either but it helps avoid such issues in the future.

@mhdawson
Copy link
Member Author

mhdawson commented May 11, 2018 •
edited
Loading

@bnoordhuis it should fix it for N-API addons since the inline should be behind the N-API c functions so I'd like to get it landed and do the refactoring as a follow on.

@BridgeAR
Copy link
Member

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 13, 2018
@addaleax
Copy link
Member

Landed in 1248ce1

@addaleax addaleax closed this May 14, 2018
addaleax pushed a commit that referenced this pull request May 14, 2018
@mhdawson @addaleax
async_wrap: fix memory leak in AsyncResource
1248ce1 
Reset the persistent that keeps the resource
Object alive when the AsyncResource is being
destroyed.

Fixes: nodejs/node-addon-api#237

PR-URL: #20668
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request May 14, 2018
@mhdawson @addaleax
async_wrap: fix memory leak in AsyncResource
e61337d 
Reset the persistent that keeps the resource
Object alive when the AsyncResource is being
destroyed.

Fixes: nodejs/node-addon-api#237

PR-URL: #20668
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax mentioned this pull request May 14, 2018
Merged
addaleax added a commit that referenced this pull request May 14, 2018
@addaleax
2018-05-15, Version 10.2.0 (Current), @???
3b86468 
Notable Changes:

* **addons**:
  - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668)
* **assert**:
  - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485)
* **crypto**:
  - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039)
* **http**:
  - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611)
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (helloshuangzi) [#20639](#20639)
  - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377)
  - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541)
* **timers**:
  - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298)

PR-URL: #20724
@mhdawson
Copy link
Member Author

@addaleax thanks !

addaleax added a commit that referenced this pull request May 15, 2018
@addaleax
2018-05-15, Version 10.2.0 (Current), @???
411ecf2 
Notable Changes:

* **addons**:
  - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668)
* **assert**:
  - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485)
* **crypto**:
  - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039)
* **http**:
  - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611)
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (helloshuangzi) [#20639](#20639)
  - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377)
  - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541)
* **esm**:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) [#20403](#20403)
* **timers**:
  - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298)

PR-URL: #20724
addaleax added a commit that referenced this pull request May 15, 2018
@addaleax
2018-05-15, Version 10.2.0 (Current), @???
d0eec7b 
Notable Changes:

* **addons**:
  - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668)
* **assert**:
  - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485)
* **crypto**:
  - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039)
* **http**:
  - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611)
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (Allen Yonghuang Wang) [#20639](#20639)
  - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377)
  - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541)
* **esm**:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) [#20403](#20403)
* **timers**:
  - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298)

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
d765fb6 
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 23, 2018
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
f6b6b3f 
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 23, 2018
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
5cbb905 
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 23, 2018
@addaleax @MylesBorins
2018-05-23, Version 10.2.0 (Current)
82f1811 
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
@mhdawson mhdawson mentioned this pull request Jul 4, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Jul 9, 2018
@mhdawson @MylesBorins
async_wrap: fix memory leak in AsyncResource
d7d5f11 
Reset the persistent that keeps the resource
Object alive when the AsyncResource is being
destroyed.

Fixes: nodejs/node-addon-api#237

PR-URL: #20668
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 10, 2018
@mhdawson @MylesBorins
async_wrap: fix memory leak in AsyncResource
90cde52 
Reset the persistent that keeps the resource
Object alive when the AsyncResource is being
destroyed.

Fixes: nodejs/node-addon-api#237

PR-URL: #20668
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@mhdawson @rvagg
async_wrap: fix memory leak in AsyncResource
8f5e991 
Reset the persistent that keeps the resource
Object alive when the AsyncResource is being
destroyed.

Fixes: nodejs/node-addon-api#237

PR-URL: #20668
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@mhdawson mhdawson deleted the async-leak branch September 30, 2019 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu approved these changes

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@AndreasMadsen AndreasMadsen Awaiting requested review from AndreasMadsen

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mhdawson @addaleax @bnoordhuis @BridgeAR @jasnell @gabrielschulhof @TimothyGu @MylesBorins @nodejs-github-bot