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

Deinitialization with dev as base #67

Merged
merged 1 commit into from
Feb 15, 2018
Merged

Deinitialization with dev as base #67

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4629,6 +4629,14 @@ class CmdArgs {
std::vector<const char*> argument_pointers_;
};

class HandleScopeHeapWrapper {
public:
explicit HandleScopeHeapWrapper(v8::Isolate* isolate)
: scope_(isolate) { }
private:
HandleScope scope_;
};

ArrayBufferAllocator* allocator;
Isolate::CreateParams params;
Locker* locker;
Expand All @@ -4641,6 +4649,7 @@ CmdArgs* cmd_args = nullptr;
bool _event_loop_running = false;
v8::Isolate* _isolate = nullptr;
Environment* _environment = nullptr;
HandleScopeHeapWrapper* _handle_scope_wrapper = nullptr;

bool eventLoopIsRunning() {
return _event_loop_running;
Expand Down Expand Up @@ -4696,6 +4705,29 @@ int _StopEnv() {
__lsan_do_leak_check();
#endif

delete _environment;
_environment = nullptr;

delete context_scope;
context_scope = nullptr;

if (!context.IsEmpty()) {
delete *context;
context.Clear();
}

delete isolate_data;
isolate_data = nullptr;

delete _handle_scope_wrapper;
_handle_scope_wrapper = nullptr;

delete isolate_scope;
isolate_scope = nullptr;

delete locker;
locker = nullptr;

return exit_code;
}

Expand All @@ -4704,6 +4736,9 @@ void _DeleteIsolate() {
CHECK_EQ(node_isolate, _isolate);
node_isolate = nullptr;
_isolate->Dispose();

delete allocator;
allocator = nullptr;
}

void _DeinitV8() {
Expand Down Expand Up @@ -4770,9 +4805,7 @@ void _CreateIsolate() {
void _CreateInitialEnvironment() {
locker = new Locker(_isolate);
isolate_scope = new Isolate::Scope(_isolate);
// TODO(jh): Once we write a Deinit(), we need to put this on the heap
// to call the deconstructor.
static HandleScope handle_scope(_isolate);
_handle_scope_wrapper = new HandleScopeHeapWrapper(_isolate);

isolate_data = new IsolateData(
_isolate,
Expand Down Expand Up @@ -4910,6 +4943,8 @@ int Deinitialize() {

deinitialize::_DeinitV8();

// TODO(js): Do we need to tear down OpenSsl?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am by no means an expert on OpenSSL, but according to their wiki, there are some cleanup mechanisms we should look into.
Unfortunately I don't know how node uses OpenSSL, so I have no idea which parts of the cleanup from the wiki would be necessary, but I think we should investigate this to avoid any memory leaks we can (we will have to live with some introduced by OpenSSL).
Feel free to place this into a new issue, as I think it'd break the size of the issue this PR is dealing with.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I created an issue for this: #71


deinitialize::_DeleteCmdArgs();

return exit_code;
Expand Down