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

Implement asynchronous responses #11

Merged
merged 8 commits into from
Dec 11, 2018
Merged

Conversation

ebkalderon
Copy link
Contributor

@ebkalderon ebkalderon commented Dec 8, 2018

Added

  • Create ResponseFuture and PreResolving types.
  • Add wait_for_response() methods to certain futures, which drives them to completion using a tokio::Runtime and returns a Result, restoring the original synchronous behavior.

Changed

  • Make Client, Connector, and Resolver fully asynchronous.
  • Update unit tests and get.rs example to use wait_for_response().
  • Add Send + Sync bounds on DnsResolver trait.
  • Change DnsResolver to return a Resolving future.
  • Removed some unnecessary clones by taking ownership instead of borrowing.
  • Add #[must_use] attribute to all futures.
  • Flesh out doc comments.
  • Run cargo fmt on the codebase.

Removed

  • Delete unused AsyncResolver and AsyncResolverFuture types.
  • Remove dependency on trust-dns-proto crate.

Closes #10.

@ebkalderon ebkalderon mentioned this pull request Dec 8, 2018
@kpcyrd
Copy link
Owner

kpcyrd commented Dec 10, 2018

@ebkalderon very nice work!

I've changed one of the projects using chrootable-https to your branch and everything worked out of the box after adding .wait_for_response() a few times.

I've pushed a branch called async that contains one additional commit to remove the pre_resolve workaround that's not needed anymore due to your changes. Could you review if that change works for you?

@ebkalderon
Copy link
Contributor Author

Looks good to me! I've pulled your change to my branch to get it tested by Travis, just so it's ready to merge.

One thing that's missing in this PR but I think might eventually make for a nice addition is the ability to specify a custom executor for the connector, instead of relying on the 4 default threads created by the HttpsConnector, but this relies on rustls/hyper-rustls#55 being resolved first. If hyper-rustls gets updated, I may make a follow-up PR, if you're interested.

@kpcyrd kpcyrd merged commit 7658bed into kpcyrd:master Dec 11, 2018
@kpcyrd
Copy link
Owner

kpcyrd commented Dec 11, 2018

@ebkalderon thanks for all your work!

@ebkalderon ebkalderon deleted the async-responses branch December 11, 2018 08:17
@ebkalderon
Copy link
Contributor Author

@kpcyrd No problem! Thanks for the very useful library.

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.

2 participants