Skip to content

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 <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Aug 12, 2017
Ref: #14738
PR-URL: #14743
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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