-
Notifications
You must be signed in to change notification settings - Fork 239
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
Switch to reqwest for proxy support #809
Conversation
let response_text = response | ||
.text() | ||
.with_context(registry_fetch_error("Node", url))?; | ||
let mut response_text = String::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I struggled with this code here. The goal is to make a blocking request and get both the response body and also the response headers which is needed lower in the file. Let me know if there's a better approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the lack of a way to destructure the Response
makes this tricky. I think the easiest approach might be to determine the expires
string before reading the data with something like:
let expires = if let Ok(expires_header) = response.headers().decode::<Expires>() {
expires_header.to_string()
} else {
let expiry_date = SystemTime::now() + Duration::from_secs(max_age(&response).into());
HttpDate::from(expiry_date).to_string()
};
Then we can use:
let response_text = response.text().with_context(registry_fetch_error("Node", url))?;
Here, and finally do write!(expiry_file, "{}", expires).with_context(...)
at the end of the file. So we calculate everything we need from the response first, then consume it to get the raw text, and then use the pre-calculated bit later on when we need it.
Thanks for this @gregjopa! I'll take a look later today! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great @gregjopa, thanks! A couple of comments on making sure to preserve the existing error behavior and a slightly modified approach to the destructuring problem you noted in node/resolve.rs
let response_text = response | ||
.text() | ||
.with_context(registry_fetch_error("Node", url))?; | ||
let mut response_text = String::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the lack of a way to destructure the Response
makes this tricky. I think the easiest approach might be to determine the expires
string before reading the data with something like:
let expires = if let Ok(expires_header) = response.headers().decode::<Expires>() {
expires_header.to_string()
} else {
let expiry_date = SystemTime::now() + Duration::from_secs(max_age(&response).into());
HttpDate::from(expiry_date).to_string()
};
Then we can use:
let response_text = response.text().with_context(registry_fetch_error("Node", url))?;
Here, and finally do write!(expiry_file, "{}", expires).with_context(...)
at the end of the file. So we calculate everything we need from the response first, then consume it to get the raw text, and then use the pre-calculated bit later on when we need it.
Thanks for the feedback @charlespierce! Your detailed comments are helping me learn Rust 😄 I think I was able to address everything. Please have another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks @gregjopa!
While I'm sad to lose the binary-size improvements from using attohttpc
, I don't find any significant difference in the startup time for the hot-path shims, so it doesn't appear to impact the performance at all.
This PR changes the http library from
attohttpc
toreqwest
for proxy support to resolve #798.The reqwest library supports proxies by default and can be set using environment variables. Ex: