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

Resolve hostnames asynchronously #12

Merged
merged 3 commits into from
Dec 7, 2019
Merged

Resolve hostnames asynchronously #12

merged 3 commits into from
Dec 7, 2019

Conversation

ebroto
Copy link
Collaborator

@ebroto ebroto commented Nov 20, 2019

Resolves #6

Thanks for the tool, I love it!

So, in addition of resolving the hostnames in a thread pool I went for making it asynchronous. It should be more performant as we will not block the threads waiting for the response from the dns servers. Let me know if you like the approach.

I have doubts about some changes I made, I will leave some comments.

ebroto
ebroto commented Nov 20, 2019
View reviewed changes
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/network/resolver.rs Outdated Show resolved Hide resolved
@imsnif
Copy link
Owner

imsnif commented Nov 21, 2019

This looks great! I'll go over it in the next few days.

@Louis-Lesage Louis-Lesage mentioned this pull request Nov 21, 2019
Closed
imsnif
imsnif requested changes Nov 23, 2019
View reviewed changes
Copy link
Owner

@imsnif imsnif left a comment

Choose a reason for hiding this comment

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

This is really great work. Looks like you put a lot of effort in here - I really like it and would love to see it merged ASAP.
Once the issues in the comments are addressed, I'd be happy to merge this. If at any point you find you don't have enough time, or are stuck - please let me know. I'd be happy to discuss solutions, alternatives, or to dive in and do some of the work as well.
Thanks for this! Looking forward to continuing the merge.

Cargo.toml Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/network/resolver.rs Outdated Show resolved Hide resolved
src/network/resolver.rs Outdated Show resolved Hide resolved
src/network/resolver.rs Outdated Show resolved Hide resolved
src/network/resolver.rs Outdated Show resolved Hide resolved
src/tests/fakes/fake_input.rs Outdated Show resolved Hide resolved
@ebroto
Copy link
Collaborator Author

ebroto commented Nov 28, 2019

Hey, just a heads-up, I'm waiting for this issue to be solved. Tokio 0.2.0 was released two days ago and broke a bit the ecosystem, dependent crates are catching up.

@imsnif
Copy link
Owner

imsnif commented Nov 29, 2019 •
edited
Loading

Thanks for the update! (and no rush, of course)

@ebroto ebroto requested a review from imsnif November 29, 2019 22:49
@ebroto
Copy link
Collaborator Author

ebroto commented Nov 29, 2019

It seems that the issue I mentioned can't be fixed until next week because some dependency is blocking it. For the moment I've done as the author suggests and I'm using the branch that will be merged when the issue is solved.

We should either not merge this PR until the issue is solved or change the version in Cargo.toml as soon as a new one is available (remember that you can't cargo publish with a git dependency).

I think I've dealt with all the suggestions, let me know what you think!

@ebroto
Copy link
Collaborator Author

ebroto commented Nov 30, 2019

Hey, just squashed and rebased on top of the new changes in master to solve the conflicts

@imsnif
Copy link
Owner

imsnif commented Nov 30, 2019

You are extremely fast :D Still merging

@imsnif
Copy link
Owner

imsnif commented Nov 30, 2019

The changes look great. I especially liked the naming (eg. dns_client), your solution with the cache method and the Lookup trait. Good stuff.

I also took it out for a spin and it works really fast!

About the trust-dns-resolver issue: I have managed in the past to publish to cargo with a git dependency. All it required was to add the version as well as the url to Cargo.toml (though that was a dev dependency, so I wonder if it will also work here pondering eomji). I wouldn't want to do that if it can be avoided though (because github is a mutable medium).

Let's give it a week until we merge and if it's not over by then, we'll think up another solution for it. Sounds okay?

Meanwhile I'm going to hold off on releasing the other changes in master.

@imsnif imsnif mentioned this pull request Nov 30, 2019
Merged
@ebroto
Copy link
Collaborator Author

ebroto commented Nov 30, 2019

Sounds good to me!

@ebroto
fix(package): update trust-dns and async-trait
2cd4ef5 
This fixes the build problems with the previous latest trust-dns version from
crates.io
@ebroto
fix(dns): use more idiomatic cloned in FakeResolver
f449555
@ebroto
Copy link
Collaborator Author

ebroto commented Dec 6, 2019

A new version of trust-dns was released 🎉

@ebroto ebroto mentioned this pull request Dec 6, 2019
Closed
@imsnif imsnif merged commit 307162b into imsnif:master Dec 7, 2019
@ebroto ebroto deleted the feature/async-dns branch December 7, 2019 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imsnif imsnif imsnif approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Faster DNS resolution
2 participants
@ebroto @imsnif