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: refactor cares_wrap internals #38172

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 9, 2021

This is the first step of a significant refactoring of the DNS subsystem. The first step is to simplify and modernize the code structure, eliminate some duplicate code, and apply some of the more modern patterns that we have implemented elsewhere in the code.

Additional refactorings will come in additional PRs.

Signed-off-by: James M Snell jasnell@gmail.com

@jasnell jasnell requested a review from addaleax April 9, 2021 19:36
@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. needs-ci PRs that need a full CI run. labels Apr 9, 2021
@jasnell jasnell added the dns Issues and PRs related to the dns subsystem. label Apr 9, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@targos
Copy link
Member

targos commented Apr 11, 2021

I guess this is now in conflict with #38184 ?

@XadillaX
Copy link
Contributor

I guess this is now in conflict with #38184 ?

same question too.

@jasnell
Copy link
Member Author

jasnell commented Apr 11, 2021

No. I've talked to @devsnek about it. That other pr is semver major and makes a number of larger breaking changes. This is semver-patch and is good for everything >= 16.x.

@addaleax
Copy link
Member

@jasnell So, just practically speaking … it’s virtually impossible to review PRs that move large chunks of code around and make some minor adjustments in a bunch of places. I’m not sure how to progress here.

@targos
Copy link
Member

targos commented Apr 12, 2021

I'm like Anna, I don't know how to tackle this review.

@jasnell
Copy link
Member Author

jasnell commented Apr 12, 2021

I'd look at it as a new contribution, largely ignoring the old bits that were changed, focus on the new bits. There's only one test change because one of the internal classes was renamed. There should be absolutely no change in actual behavior.

@addaleax
Copy link
Member

@jasnell I think in that case there will be a lot of comments … but yeah, I can do that sometime this week

@jasnell
Copy link
Member Author

jasnell commented Apr 12, 2021

The changes that were made are:

  • Class declarations were removed from node_cares.cc and moved into a new node_cares.h
  • node_ares_task was renamed to NodeAresTask and the associated Hash and Equal impls were moved into NodeAresTask.
  • node_ares_task_list was changed to a using List inside NodeAresTask.
  • class ChannelWrap : => class ChannelWrap final :
  • class GetAddrInfoReqWrap : => class GetAddrInfoReqWrap final :
  • class GetNameInfoReqWrap : => class GetNameInfoReqWrap final :
  • class QueryWrap was changed into a final template class. Rather than being subclassed for each individual query type, a using ... = QueryWrap<...> pattern is used for each type, matching the changes that have been made elsewhere in the codebase (e.g. in crypto).
  • Added the struct QueryTraits template to allow eliminating duplicated code (previously, every query type subclass had nearly identical implementations of Send().
  • Eliminated a dead code path (an unused override of Send() that was in every query type subclass.
  • Introduced the HostentPointer smart pointer (using DeleteFnPtr) to make cleanup of hostent* a bit nicer
  • Other small cleanups to match formatting and eliminate unnecessary assignments (e.g. removing auto context = env->context() calls and replacing context with env->context().

There should be no observable functional changes with the exception of the rename of node_ares_task_list to NodeAresTask::List in a generated heapdump. All existing dns related tests and benchmarks should continue to operate correctly without modification.

@jasnell
Copy link
Member Author

jasnell commented Apr 12, 2021

@addaleax

I think in that case there will be a lot of comments

I would expect nothing less ;-)

This particular bit of code has always been far from perfect and I have many more changes in the works to improve it so a detailed review is helpful.

src/cares_wrap.h Show resolved Hide resolved
src/cares_wrap.h Show resolved Hide resolved
src/cares_wrap.cc Show resolved Hide resolved
src/cares_wrap.cc Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 14, 2021
src/cares_wrap.h Show resolved Hide resolved
src/cares_wrap.h Outdated Show resolved Hide resolved
src/cares_wrap.cc Show resolved Hide resolved
@jasnell jasnell removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 14, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 15, 2021

@jasnell
Copy link
Member Author

jasnell commented Apr 16, 2021

@addaleax ... this is generally ready to go. Did you have any more feedback on it before I land later today?

@jasnell jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Apr 16, 2021
@addaleax
Copy link
Member

@jasnell No, feel free to merge 👍

@jasnell
Copy link
Member Author

jasnell commented Apr 16, 2021

Awesome ok. I know there's still a lot in this code that can be improved but with the possible move to getdns in the work, I'm not sure how much more effort we should put into it. I'll merge later on this afternoon. (although, I guess since the getdns change is semver major, and 16 will be around for a while, it would at least make sense to explore some good perf improvements here still)

Signed-off-by: James M Snell <jasnell@gmail.com>
jasnell added a commit that referenced this pull request Apr 17, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #38172
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Apr 17, 2021

Landed in bc31dc0

@jasnell jasnell closed this Apr 17, 2021
targos pushed a commit that referenced this pull request May 1, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #38172
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
danielleadams pushed a commit that referenced this pull request May 8, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #38172
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request May 14, 2021
codebytere added a commit to electron/electron that referenced this pull request May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. dns Issues and PRs related to the dns subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants