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

Deinitialization with dev as base #67

Merged
merged 1 commit into from
Feb 15, 2018
Merged

Conversation

Johannes-Schneider
Copy link
Collaborator

Added code from initial PR (#62)

Copy link
Collaborator

@justus-hildebrand justus-hildebrand left a comment

Choose a reason for hiding this comment

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

LGTM. When merging this, remember to open a new issue for the OpenSSL deinitialization.

@@ -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

@Johannes-Schneider Johannes-Schneider merged commit b2a8f79 into dev Feb 15, 2018
@Johannes-Schneider Johannes-Schneider deleted the deinitialization branch February 15, 2018 10:15
msoechting pushed a commit that referenced this pull request Feb 21, 2018
Deinitialization with dev as base
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants