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

memleak using worker and objects wrapping c++ instances #38816

Closed
julianjany opened this issue May 26, 2021 · 5 comments
Closed

memleak using worker and objects wrapping c++ instances #38816

julianjany opened this issue May 26, 2021 · 5 comments
Labels
addons Issues and PRs related to native addons. c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations. memory Issues and PRs related to the memory management or memory footprint. worker Issues and PRs related to Worker support.

Comments

@julianjany
Copy link
Contributor

  • Version: 16.2.0
  • Platform: Windows 10 x86-64 (most likely platform-independent)
  • Subsystem: Workers

What steps will reproduce the bug?

Sample description

provided reproducer-sample: gist

The main thread continuously instantiates worker-threads.
Inside of these workers an instance of a wrapped C++ object is created (see Wrapping C++ objects).
These wrapped objects are not properly destroyed on worker-shutdown (assuming garbage-collection is not manually triggered before shutdown), causing a memory-leak.

Steps to reproduce the bug locally

  • install node-gyp if not already installed → run: npm i -g node-gyp
  • clone the following gist
  • open the project-root-folder
  • run: node-gyp configure
  • run: node-gyp build
  • execute the example → run: node --expose-gc ./testAddon.js

How often does it reproduce? Is there a required condition?

always. no precondition.

What is the expected behavior?

Proper destruction of the wrapped objects.

expected console-output:

...
worker startup
native: MyObject()
native: ~MyObject()
worker shutdown
...

What do you see instead?

The wrapped objects are not destroyed and cause a memory-leak.

perceived console-output:

...
worker startup
native: MyObject()
worker shutdown
...

The instantiated objects allocate a large integer vector, so the effects of the memory-leak are also easily visible in "Task Manager".

Additional information

The same behavior also applies to wrapped objects instantiated inside the main thread.
There the problem is not as severe because the memory is "handed back" to the OS on process-shutdown.
A C++ developer developing a native addon still expects the destructor of the object to be called at some point, which makes this behavior problematic.

@Flarna Flarna added worker Issues and PRs related to Worker support. c++ Issues and PRs that require attention from people who are familiar with C++. memory Issues and PRs related to the memory management or memory footprint. labels May 26, 2021
@addaleax addaleax added the addons Issues and PRs related to native addons. label May 26, 2021
@addaleax
Copy link
Member

https://nodejs.org/api/addons.html#addons_worker_support mentions what you need to do: Use AddEnvironmentCleanupHook and RemoveEnvironmentCleanupHook to manage object lifetime.

We should really just not recommend using ObjectWrap anymore, at all.

@julianjany
Copy link
Contributor Author

I am already using AddEnvironmentCleanupHook / RemoveEnvironmentCleanupHook in other places of the code. They make perfect sense for objects whose lifetime should be directly bound to the lifetime of the surrounding Isolate / Context.
To me it still seems like the behavior of ObjectWrap is incorrect.
I feel like the mentioned hooks are more of a workaround/hack than an appropriate replacement for ObjectWrap, considering this specific usecase.

@julianjany
Copy link
Contributor Author

Here is a minimal sample outlining my current workaround:

class FooWrap : public node::ObjectWrap {
public:
    FooWrap(v8::Isolate* isolate) : mIsolate{isolate} {
        node::AddEnvironmentCleanupHook(mIsolate, DeleteInstance, this);
    }

    virtual ~FooWrap() override {
        if (mIsolate != nullptr) {
            // The object is "deleted" before the cleanup-hook is called;
            // most likely due to garbage collection during runtime.
            // The hook has to be removed:
            node::RemoveEnvironmentCleanupHook(mIsolate, DeleteInstance, this);
        } else {
            // DeleteInstance(..) was called during the environment cleanup process.
        }
    }

private:
    static void DeleteInstance(void* data) {
        FooWrap* wrapped = static_cast<FooWrap*>(data);
        wrapped->mIsolate = nullptr;
        delete wrapped;
    }

    v8::Isolate* mIsolate;
};

As mentioned in my last comment this seems rather "hacky" to me.
Also form a performance-perspective this is not ideal.
Installing hooks for potentially thousands of objects sure comes at a cost.

@addaleax
Copy link
Member

addaleax commented May 27, 2021

Installing hooks for potentially thousands of objects sure comes at a cost.

It’s the same thing Node.js’s own internal objects do – we’ve done our best to make it fast (including for this specific reason).

As mentioned in my last comment this seems rather "hacky" to me.

That’s fair, but 99 % of new addon development happens with Node-API anyway, ObjectWrap is used by basically nobody and for those who really need low-level control I think giving it to them is also not a bad thing.

@julianjany
Copy link
Contributor Author

I ported my small sample to Node-API (node-addon-api to be more precise) ... everything works as expected! 😄
Thanks for your feedback!

I still think some sort of warning/hint regarding this behavior of ObjectWrap would be helpful. 🤔

@addaleax addaleax added the doc Issues and PRs related to the documentations. label May 27, 2021
BethGriggs pushed a commit that referenced this issue Sep 21, 2021
Clarify that ObjectWrap instances are not destroyed on process or
worker shutdown and require manual destruction to avoid resource
leaks.

PR-URL: #40074
Fixes: #38816
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this issue Sep 21, 2021
Clarify that ObjectWrap instances are not destroyed on process or
worker shutdown and require manual destruction to avoid resource
leaks.

PR-URL: #40074
Fixes: #38816
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations. memory Issues and PRs related to the memory management or memory footprint. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants