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

feat: use reqwest as http client #2822

Merged
merged 2 commits into from
Aug 30, 2019
Merged

feat: use reqwest as http client #2822

merged 2 commits into from
Aug 30, 2019

Conversation

bartlomieju
Copy link
Member

Closes #2720

@bartlomieju
Copy link
Member Author

bartlomieju commented Aug 27, 2019 •
edited
Loading

I'm getting compilation errors related to mismatched crate versions. It looks like reqwest uses url crate with version 1.7.2 while in our Cargo.toml we define url with version 2.1.0.

@ry does this situation disqualify using reqwest? (I mean that I can downgrade url so there's no conflict, but are you ok with that solution?)

bartlomieju
bartlomieju commented Aug 27, 2019
View reviewed changes
cli/http_util.rs Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member Author

@ry PTAL, this is now feature complete. If you're fine with downgrading url crate then I'd ask you to add reqwest to third_party repo so CI can build with GN.

Benefits of using reqwest:

bartlomieju
bartlomieju commented Aug 28, 2019
View reviewed changes
cli/http_body.rs Show resolved Hide resolved
cli/resources.rs Outdated Show resolved Hide resolved
@ry ry requested a review from piscisaureus August 28, 2019 14:39
@ry
Copy link
Member

ry commented Aug 28, 2019

@ry PTAL, this is now feature complete. If you're fine with downgrading url crate then I'd ask you to add reqwest to third_party repo so CI can build with GN.

@piscisaureus can you run gnargo on this?

@ry
Copy link
Member

ry commented Aug 28, 2019

Does this patch add proxy support?

@bartlomieju
Copy link
Member Author

Does this patch add proxy support?

Implicitly - it's done by reqwest transparently. Tests for proxy will follow in other PR

@ry
Copy link
Member

ry commented Aug 28, 2019

Implicitly - it's done by reqwest transparently

I suppose that means there's an environmental variable? Can you add some docs for this?

@bartlomieju
Copy link
Member Author

Implicitly - it's done by reqwest transparently

I suppose that means there's an environmental variable? Can you add some docs for this?

Ah, sorry. It's not on by default:

A Client can be configured to make use of HTTP proxies by adding Proxys to a ClientBuilder.

** NOTE** System proxies will be used in the next breaking change.

I'll enable & document it in a follow up PR.

@ry
Copy link
Member

ry commented Aug 28, 2019

I'll enable & document it in a follow up PR.

I'm fine with landing this as two PRs, but I think we should see a proof-of-concept for proxy before landing this, since that's the whole purpose of reqwest (?). Maybe just do that work in this branch.

bartlomieju
bartlomieju commented Aug 28, 2019
View reviewed changes
cli/http_body.rs
@@ -87,26 +87,3 @@ impl AsyncRead for HttpBody {
}
}
}

#[test]
fn test_body_async_read() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately all methods of Decoder are private or crate scoped, so there's no way to construct it manually...

@piscisaureus
Copy link
Member

@bartlomieju Url can be downgraded back to 1.7.2 IMO.

@bartlomieju
Copy link
Member Author

@piscisaureus can you update deps in third_party?

piscisaureus
piscisaureus reviewed Aug 29, 2019
View reviewed changes
cli/Cargo.toml Outdated Show resolved Hide resolved
@piscisaureus
Copy link
Member

@bartlomieju I've added reqwest to the GN build. It builds for me but there are a few test failures.

@bartlomieju
Copy link
Member Author

@piscisaureus tests fixed, please decide on the DNS resolver (I suggest we try the new one) and review

piscisaureus added a commit to bartlomieju/deno that referenced this pull request Aug 29, 2019
@piscisaureus
Use 'reqwest' to implement HTTP client (denoland#2822)
7faaeec 
Closes denoland#2720
piscisaureus pushed a commit to bartlomieju/deno that referenced this pull request Aug 29, 2019
@bartlomieju @piscisaureus
Use 'reqwest' to implement HTTP client (denoland#2822)
d764ea8 
Closes denoland#2720
@piscisaureus piscisaureus force-pushed the feat-use_reqwest_as_http_client branch from 7faaeec to d764ea8 Compare August 29, 2019 19:12
piscisaureus pushed a commit to bartlomieju/deno that referenced this pull request Aug 29, 2019
@bartlomieju @piscisaureus
Use 'reqwest' to implement HTTP client (denoland#2822)
19839f6 
Closes denoland#2720
@piscisaureus piscisaureus force-pushed the feat-use_reqwest_as_http_client branch from d764ea8 to 19839f6 Compare August 29, 2019 19:45
piscisaureus
piscisaureus reviewed Aug 29, 2019
View reviewed changes
cli/file_fetcher.rs
@@ -539,11 +531,6 @@ fn filter_shebang(bytes: Vec<u8>) -> Vec<u8> {
}
}

fn url_into_uri(url: &url::Url) -> http::uri::Uri {
Copy link
Member

Choose a reason for hiding this comment

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

Nice to see this go!

piscisaureus
piscisaureus approved these changes Aug 29, 2019
View reviewed changes
Copy link
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

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

LGTM.
@ry I noticed you had some comments about a POC HTTP proxy, but that isn't implemented at this point.
You still want to wait for that?

@ry ry mentioned this pull request Aug 29, 2019
Closed
@bartlomieju
Copy link
Member Author

@piscisaureus proxy support is enabled - see get_client function. There are no tests yet, I'll add them once I'm back

@ry
Copy link
Member

ry commented Aug 29, 2019

If it can go green I’m ok with landing it

@piscisaureus piscisaureus force-pushed the feat-use_reqwest_as_http_client branch from 19839f6 to 723284f Compare August 30, 2019 21:49
@piscisaureus piscisaureus merged commit 723284f into denoland:master Aug 30, 2019
@piscisaureus
Copy link
Member

@bartlomieju, @ry

I landed this patch without the trust-dns feature, because with that feature the tests weren't passing.

The patch that adds trust-dns (master...piscisaureus:reqwest_trust_dns) still applies, so we can land that at a later time if the tests can be made to go green.

@ry
Copy link
Member

ry commented Aug 30, 2019

This patch added 1 megabyte to the executable.

Screen Shot 2019-08-30 at 7 45 46 PM

@bartlomieju bartlomieju mentioned this pull request Sep 1, 2019
Merged
@bartlomieju bartlomieju deleted the feat-use_reqwest_as_http_client branch September 7, 2019 12:24
@bartlomieju bartlomieju mentioned this pull request Oct 5, 2019
Closed
@trivikr trivikr mentioned this pull request Jan 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piscisaureus piscisaureus piscisaureus approved these changes

@ry ry Awaiting requested review from ry

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

Successfully merging this pull request may close these issues.

Consider reqwest as internal HTTP client
3 participants
@bartlomieju @ry @piscisaureus