Remove WrappedHttpResponse abstraction #19369

tigercosmos · 2017-11-24T20:17:37Z

Remove WrappedHttpResponse abstraction, and use HyperResponse

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Remove WrappedHttpResponse abstraction #19361 (github issue number if applicable).

This change is

highfive · 2017-11-24T20:17:41Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/lib.rs, components/net/http_loader.rs

highfive · 2017-11-24T20:17:43Z

Warning

These commits modify net code, but no tests are modified. Please consider adding a test!

jdm · 2017-11-24T20:20:53Z

components/net/lib.rs

@@ -1,7 +1,7 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
+#![feature(type_ascription)]


This should not be necessary.

jdm · 2017-11-24T20:21:33Z

components/net/http_loader.rs

@@ -499,8 +480,7 @@ fn obtain_response(connector: &Pool<Connector>,
            debug!("Not notifying devtools (no request_id)");
            None
        };
-
-        return Ok((WrappedHttpResponse { response: response }, msg));
+        return Ok(({ response: HyperResponse }, msg));


I was surprised that this builds, but I guess the type_ascription feature makes it valid. In any case, we should just use Ok((response, msg)) instead.

jdm · 2017-11-24T20:24:03Z

components/net/http_loader.rs

+    fn from_http_response(response: HyperResponse) -> io::Result<StreamedResponse> {
+        let mut encode_type = "";
+        let decoder;
+        if let Some(encoding) = response.headers.get::<ContentEncoding>() {


Let's do this instead:

let decoder = if let Some(encoding) = response.headers.get::<ContentEncoding>() { if encoding.contains(&Encoding::Gzip) { Decoder::Gzip(GzDecoder::new(response)?) } else if encoding.contains(...) { ... } } else { Decoder::Plain(response) };

@jdm
this will cause borrow issue
I don't know how to solve it

error[E0505]: cannot move out of `response` because it is borrowed --> components/net/http_loader.rs:297:61 | 291 | if let Some(encoding) = response.headers.get::<ContentEncoding>(){ | ---------------- borrow of `response.headers` occurs here ... 297 | decoder = Decoder::Brotli(Decompressor::new(response, 1024)); | ^^^^^^^^ move out of `response` occurs here

Use ref encoding.

Sorry for not familiar with Rust...

@jdm Do you mean this?

impl StreamedResponse { fn from_http_response(response: HyperResponse) -> io::Result<StreamedResponse> { let decoder; if let Some(ref encoding) = response.headers.get::<ContentEncoding>() { if encoding.contains(&Encoding::Gzip) { decoder = Decoder::Gzip(GzDecoder::new(response)?); } else if encoding.contains(&Encoding::Deflate) { decoder = Decoder::Deflate(DeflateDecoder::new(response)); } else if encoding.contains(&Encoding::EncodingExt("br".to_owned())) { decoder = Decoder::Brotli(Decompressor::new(response, 1024)); } } else { decoder = Decoder::Plain(response); } Ok(StreamedResponse { decoder: decoder }) } }

it will get

error[E0505]: cannot move out of `response` because it is borrowed --> components/net/http_loader.rs:293:56 | 291 | if let Some(ref encoding) = response.headers.get::<ContentEncoding>() { | ---------------- borrow of `response.headers` occurs here 292 | if encoding.contains(&Encoding::Gzip) { 293 | decoder = Decoder::Gzip(GzDecoder::new(response)?); | ^^^^^^^^ move out of `response` occurs here error[E0505]: cannot move out of `response` because it is borrowed --> components/net/http_loader.rs:295:64 | 291 | if let Some(ref encoding) = response.headers.get::<ContentEncoding>() { | ---------------- borrow of `response.headers` occurs here ... 295 | decoder = Decoder::Deflate(DeflateDecoder::new(response)); | ^^^^^^^^ move out of `response` occurs here error[E0505]: cannot move out of `response` because it is borrowed --> components/net/http_loader.rs:297:61 | 291 | if let Some(ref encoding) = response.headers.get::<ContentEncoding>() { | ---------------- borrow of `response.headers` occurs here ... 297 | decoder = Decoder::Brotli(Decompressor::new(response, 1024)); | ^^^^^^^^ move out of `response` occurs here error[E0505]: cannot move out of `response` because it is borrowed --> components/net/http_loader.rs:300:38 | 291 | if let Some(ref encoding) = response.headers.get::<ContentEncoding>() { | ---------------- borrow of `response.headers` occurs here ... 300 | decoder = Decoder::Plain(response); | ^^^^^^^^ move out of `response` occurs here error[E0381]: use of possibly uninitialized variable: `decoder` --> components/net/http_loader.rs:302:40 | 302 | Ok(StreamedResponse { decoder: decoder }) | ^^^^^^^ use of possibly uninitialized `decoder`

The original implement avoid to call response in a "response calling".
But this method will encounter this problem.

Ah, I see. In that case, instead of using strings let's use a simple enum DecoderType { Gzip, Deflate, Brotli, Plain }.

I think calling .clone() on response.headers.get::<ContentEncoding>() should solve the problem.

True, but no need to clone the whole header.

tigercosmos · 2017-11-25T03:54:27Z

@jdm This time should be OK

jdm · 2017-11-25T14:51:37Z

components/net/http_loader.rs

-            _ => {
+    fn from_http_response(response: HyperResponse) -> io::Result<StreamedResponse> {
+        let decoder = {
+            let headers = response.headers.clone();


Cloning all of the headers associated with this response is inefficient. Let's do headers.get::<ContentEncoding>().cloned() instead.

@jdm If I change to

impl StreamedResponse { fn from_http_response(response: HyperResponse) -> io::Result<StreamedResponse> { let decoder = { let contentEncoding = response.headers.get::<ContentEncoding>().clone(); if let Some(ref encoding) = contentEncoding { if encoding.contains(&Encoding::Gzip) { Decoder::Gzip(GzDecoder::new(response)?)

Error again:

error[E0505]: cannot move out of `response` because it is borrowed --> components/net/http_loader.rs:294:50 | 291 | let contentEncoding = response.headers.get::<ContentEncoding>().clone(); | ---------------- borrow of `response.headers` occurs here ... 294 | Decoder::Gzip(GzDecoder::new(response)?) | ^^^^^^^^ move out of `response` occurs here

if change to

let contentEncoding = response.headers.clone().get::<ContentEncoding>();

the lift time will be not long enough.

I did say cloned instead of clone. Does that make a difference?

sorry :(
I didn't read carefully.

jdm · 2017-11-25T16:56:03Z

@bors-servo: r+

bors-servo · 2017-11-25T16:56:04Z

📌 Commit 2e3d1d8 has been approved by jdm

bors-servo · 2017-11-25T16:56:09Z

⌛ Testing commit 2e3d1d8 with merge adfd51c...

Remove WrappedHttpResponse abstraction  Remove `WrappedHttpResponse` abstraction, and use `HyperResponse` ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #19361 (github issue number if applicable).    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19369)

bors-servo · 2017-11-25T17:48:40Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing adfd51c to master...

tigercosmos · 2017-11-25T17:56:54Z

thanks @jdm @KiChjang

highfive assigned jdm Nov 24, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 24, 2017

jdm requested changes Nov 24, 2017

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 24, 2017

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 24, 2017

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 24, 2017

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 25, 2017

jdm requested changes Nov 25, 2017

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 25, 2017

Remove WrappedHttpResponse abstraction

2e3d1d8

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 25, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 25, 2017

bors-servo merged commit 2e3d1d8 into servo:master Nov 25, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 25, 2017

tigercosmos deleted the a1 branch November 25, 2017 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove WrappedHttpResponse abstraction #19369

Remove WrappedHttpResponse abstraction #19369

tigercosmos commented Nov 24, 2017 •

edited

Loading

highfive commented Nov 24, 2017

highfive commented Nov 24, 2017

jdm Nov 24, 2017

jdm Nov 24, 2017

jdm Nov 24, 2017

tigercosmos Nov 24, 2017

jdm Nov 24, 2017

tigercosmos Nov 24, 2017

tigercosmos Nov 24, 2017 •

edited

Loading

jdm Nov 24, 2017

KiChjang Nov 24, 2017

jdm Nov 25, 2017

tigercosmos commented Nov 25, 2017

jdm Nov 25, 2017

tigercosmos Nov 25, 2017

tigercosmos Nov 25, 2017 •

edited

Loading

jdm Nov 25, 2017

tigercosmos Nov 25, 2017

jdm commented Nov 25, 2017

bors-servo commented Nov 25, 2017

bors-servo commented Nov 25, 2017

bors-servo commented Nov 25, 2017

tigercosmos commented Nov 25, 2017

Remove WrappedHttpResponse abstraction #19369

Remove WrappedHttpResponse abstraction #19369

Conversation

tigercosmos commented Nov 24, 2017 • edited Loading

highfive commented Nov 24, 2017

highfive commented Nov 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigercosmos Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigercosmos commented Nov 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigercosmos Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm commented Nov 25, 2017

bors-servo commented Nov 25, 2017

bors-servo commented Nov 25, 2017

bors-servo commented Nov 25, 2017

tigercosmos commented Nov 25, 2017

tigercosmos commented Nov 24, 2017 •

edited

Loading

tigercosmos Nov 24, 2017 •

edited

Loading

tigercosmos Nov 25, 2017 •

edited

Loading