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

dns: make library_inited_ state global #14738

Closed

Conversation

XadillaX
Copy link
Contributor

@XadillaX XadillaX commented Aug 10, 2017

Once ares_library_init(ARES_LIB_INIT_ALL) succeeded, it shouldn't be
initialized again. So make the flag library_inited_ global.

Checklist
Affected core subsystem(s)

dns, c-ares

Once `ares_library_init(ARES_LIB_INIT_ALL)` succeeded, it shouldn't be
initialized again. So make the flag `library_inited_` global.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. labels Aug 10, 2017
@addaleax
Copy link
Member

Once ares_library_init(ARES_LIB_INIT_ALL) succeeded, it shouldn't be initialized again.

Just in case it isn’t obvious, ares_library_init is a no-op if the library is already initialized.

@XadillaX
Copy link
Contributor Author

@addaleax but in semantic I think making it global is better.

@XadillaX
Copy link
Contributor Author

What's more, I think we shoudn't clean the environment up in ~ChannelWrapper

@addaleax
Copy link
Member

What's more, I think we shoudn't clean the environment up in ~ChannelWrapper

Well, for the current approach that’s what makes sense; init in the constructor, cleanup in the destructor.

I don’t oppose this change, it’s just chaining two pieces of global state instead of having a single one, but I don’t think I’m +1 on it, just because global state generally is an antipattern. It happens to make no difference for Node as it is because there’s always a single ChannelWrap instance around if DNS has been require()d, but if that were not the case, this would introduce a resource leak.

@XadillaX
Copy link
Contributor Author

@addaleax Sorry but I just saw the source code of C-ARES. It will plus ares_initialized when ares_library_init() and minus one when ares_library_cleanup().

So I think this PR could be closed now.

@addaleax
Copy link
Member

@XadillaX That’s your call. But I can see why what I wrote might be confusing; maybe it’s better to add comments explaining why the current code is fine?

@XadillaX
Copy link
Contributor Author

@addaleax Yes I think so. Because when I saw the name of the function ares_library_init without reading the source code of it, I was thinking about that this function should be called only once in the whole process.

@addaleax
Copy link
Member

@XadillaX Okay; do you want to add the comments or should I?

@XadillaX
Copy link
Contributor Author

XadillaX commented Aug 10, 2017

@addaleax I think you're better than me because my English is not that good. It'll take a long time to review if I do the comment.

addaleax added a commit to addaleax/node that referenced this pull request Aug 10, 2017
@XadillaX XadillaX closed this Aug 11, 2017
addaleax added a commit that referenced this pull request Aug 11, 2017
Ref: #14738
PR-URL: #14743
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Aug 12, 2017
Ref: #14738
PR-URL: #14743
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Colin Ihrig <[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
c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants