(WIP) Make fetch happen #11439

Manishearth · 2016-05-26T11:12:35Z

This adds a FetchTaskTarget, but doesn't use it yet. Further commits will make
XHR use the fetch task target.

@jdm thoughts so far?

This change is

highfive · 2016-05-26T11:12:38Z

Heads up! This PR modifies the following files:

@KiChjang: components/net_traits/response.rs, components/net_traits/response.rs, components/net/fetch/methods.rs, components/net_traits/request.rs, components/net_traits/request.rs, components/net_traits/lib.rs, components/net_traits/lib.rs

highfive · 2016-05-26T11:12:39Z

Warning

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

KiChjang · 2016-05-26T14:56:01Z

I suggest trying to get the unit tests working. That should give us a good idea about where your design is headed.

KiChjang · 2016-05-26T14:57:33Z

components/net_traits/request.rs

@@ -106,6 +107,25 @@ pub enum CORSSettings {
    UseCredentials
 }

+pub trait FetchTaskTarget {


I would arguably put this in net_traits/lib.rs, use net_traits::request::FetchTaskTarget doesn't quite appeal to me.

highfive · 2016-05-27T08:49:37Z

New code was committed to pull request.

Manishearth · 2016-05-27T08:50:11Z

Tests work now (there was a minor issue where it hangs on network errors, fixed and moved the channel creation to a point where there never will be a channel if there's nothing to block on)

highfive · 2016-05-27T09:31:53Z

New code was committed to pull request.

jdm · 2016-05-27T12:02:59Z

Doesn't build :)

highfive · 2016-05-27T12:19:52Z

New code was committed to pull request.

jdm · 2016-05-27T12:37:16Z

-S-awaiting-review +S-needs-code-changes

Previously, highfive wrote…

New code was committed to pull request.

Reviewed 4 of 4 files at r1.
Review status: 1 of 5 files reviewed at latest revision, 11 unresolved discussions.

components/net/fetch/methods.rs, line 260 [r1] (raw file):

    // Step 18
    if request.synchronous {
        done_chan.as_ref().map(|ref ch| ch.1.recv());

if let, although we probably want to assert that we actually have a done_chan here?

components/net/fetch/methods.rs, line 266 [r1] (raw file):

    // Step 19
    if request.body.borrow().is_some() && matches!(request.current_url().scheme(), "http" | "https") {
        target.as_mut().map(|target| {

if let

components/net/fetch/methods.rs, line 278 [r1] (raw file):

    {
        // Step 20
        target.as_mut().map(|target| target.process_response(&response));

if let

components/net/fetch/methods.rs, line 281 [r1] (raw file):

        // Step 21
        done_chan.as_ref().map(|ref ch| ch.1.recv());

We specifically need to wait for the internal response to be complete, and I find it difficult to reason about whether that's actually happening now.

components/net/fetch/methods.rs, line 284 [r1] (raw file):

        // Step 22
        target.as_mut().map(|t| t.process_response_eof(&response));

if let

components/net/fetch/methods.rs, line 445 [r1] (raw file):

        // Substep 4
        let fetch_result = http_network_or_cache_fetch(request.clone(), credentials, authentication_fetch_flag,
                                                       done_chan.as_ref().map(|ref chan| chan.0.clone()));

I'd rather have a separate local variable than have this inline here.

components/net/fetch/methods.rs, line 472 [r1] (raw file):

                },
                RedirectMode::Follow => {

Let's remove this newline.

components/net/fetch/methods.rs, line 883 [r1] (raw file):

                            };
                            *res_body.lock().unwrap() = ResponseBody::Done(completed_body);
                            done_sender.map(|s| s.send(()));

if let

components/net/fetch/methods.rs, line 893 [r1] (raw file):

        Err(_) => {
            response.termination_reason = Some(TerminationReason::Fatal);
            done_sender.map(|s| s.send(()));

if let

components/net_traits/request.rs, line 115 [r1] (raw file):

    /// Fired when a chunk of the request body is transmitted
    fn process_request_body(&mut self, request: &Request);
    /// https://fetch.spec.whatwg.org/#process-request-end-of-file

Add a newline after each method, please.

Comments from Reviewable

highfive · 2016-05-27T13:11:36Z

New code was committed to pull request.

bors-servo · 2016-05-28T00:32:41Z

☔ The latest upstream changes (presumably #11472) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2016-05-30T17:38:19Z

I still haven't thought hard about what waiting on the channel means, so leaving the awaiting-review label.
+S-needs-code-changes +S-fails-tidy

Previously, bors-servo wrote…

☔ The latest upstream changes (presumably #11472) made this pull request unmergeable. Please resolve the merge conflicts.

Reviewed 4 of 4 files at r2.
Review status: 2 of 5 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

components/net/fetch/methods.rs, line 261 [r2] (raw file):

    // Step 18
    if request.synchronous {
        if !response.is_network_error() {

Is there a test for this?

components/net_traits/lib.rs, line 160 [r2] (raw file):

    ///
    /// Fired when a chunk of the request body is transmitted
    fn process_request_body(&mut self, request: &request::Request);

Add a newline after each method, please.

tests/unit/net/fetch.rs, line 49 [r2] (raw file):

}

fn fetch_async(request: Request, target: Box<FetchTaskTarget+Send>) {

This looks like fetch_async from methods.rs. We should duplicate the useful pieces, like the thread name.

tests/unit/net/fetch.rs, line 80 [r2] (raw file):

    let wrapped_request = Rc::new(request);

    let fetch_response = fetch(wrapped_request, None);

We should define a fetch_sync method that hides the None argument, and not leave fetch publicly exposed.

Comments from Reviewable

Manishearth · 2016-05-31T09:43:53Z

Review status: 2 of 5 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

components/net/fetch/methods.rs, line 260 [r1] (raw file):

Previously, jdm (Josh Matthews) wrote…

if let, although we probably want to assert that we actually have a done_chan here?

No, done_chan can be None in any of the cases where the request doesn't need a thread to complete (cache, network error, etc)

components/net/fetch/methods.rs, line 261 [r2] (raw file):

Previously, jdm (Josh Matthews) wrote…

Is there a test for this?

For what?

Comments from Reviewable

jdm · 2016-05-31T12:28:23Z

Reviewed 1 of 1 files at r3.
Review status: 2 of 5 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

components/net/fetch/methods.rs, line 261 [r2] (raw file):

Previously, Manishearth (Manish Goregaokar) wrote…

For what?

For this new code that was added to handle an error case. What was the motivation for adding it, and can we write a test for it?

components/net/fetch/methods.rs, line 262 [r2] (raw file):

    if request.synchronous {
        if !response.is_network_error() {
            done_chan.as_ref().map(|ref ch| ch.1.recv());

if let

Comments from Reviewable

Manishearth · 2016-06-01T13:26:19Z

Most tests seem to pass, but I'm getting timeouts (which I'll go through tomorrow)

$ ./mach test-wpt --include tests/wpt/web-platform-tests/XMLHttpRequest/
Running 198 tests in web-platform-tests

Unsupported test type wdspec for product servo
  ▶ TIMEOUT [expected OK] /XMLHttpRequest/XMLHttpRequest-withCredentials.worker

STDERR: 127.0.0.1 - - [01/Jun/2016 18:48:49] code 400, message Bad request syntax ('0')
STDERR: 127.0.0.1 - - [01/Jun/2016 18:48:49] "0" 400 -
  ▶ TIMEOUT [expected OK] /XMLHttpRequest/event-readystatechange-loaded.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/interfaces.html

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/open-url-about-blank-window.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/open-url-multi-window-2.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/open-url-multi-window-3.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/open-url-multi-window-4.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/open-url-multi-window-5.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/open-url-multi-window.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/response-data-deflate.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/response-data-progress.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/response-invalid-responsetype.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/responsetype.html

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/responseurl.html

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/responsexml-document-properties.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/responsexml-media-type.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/send-authentication-basic-repeat-no-args.htm
  │ 
  └ Gtk-Message: GtkDialog mapped without a transient parent. This is discouraged.

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/send-authentication-basic-setrequestheader-existing-session.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/send-authentication-basic.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/send-content-type-string.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/send-entity-body-document.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/send-send.worker

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/send-timeout-events.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/send-usp.html

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/send-usp.worker

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/status-async.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/status-basic.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/status-error.htm

  ▶ TIMEOUT [expected OK] /XMLHttpRequest/xmlhttprequest-network-error.htm

Ran 198 tests finished in 405.0 seconds.
  • 169 ran as expected. 2 tests skipped.
  • 29 tests timed out unexpectedly

Manishearth · 2016-06-02T07:19:35Z

Sync seems to work now, though we still have behavior-related test failures

http://hastebin.com/lixazoyowo.txt

Ran 198 tests finished in 114.0 seconds.
• 172 ran as expected. 2 tests skipped.
• 2 tests timed out unexpectedly
• 26 tests had unexpected subtest results

Manishearth · 2016-06-02T10:37:04Z

Now at http://hastebin.com/lequyacuzi.txt, 3 tests fixed


        Revamp Fetch async handling to use a fetch target and done channels


        Net side of XHR fetch integration


        Abstractify the Listener abstractions further


        Hook XHR up to fetch


        Use RequestInit instead of LoadData. Make code look like the spec.


        Test fixes:

- Hack to stop hitting unreachable on referer - add fetch_done to make sync work - Make datauris work by setting the response URI, spec bug - Allow for empty bodies - Make request bodies work (pass to http, fix fencepost in iter count)


        Auto merge of #11556 - Manishearth:make-fetch-happen, r=<try>

Make fetch happen  Moves XHR over to the fetch backend. Previous PR: #11439 (recreated to get a fresh reviewable page) Doesn't yet pass all tests, mostly ready for review ---  - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors (Will fix later)  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11556)


        Auto merge of #11556 - Manishearth:make-fetch-happen, r=<try>

Make fetch happen  Moves XHR over to the fetch backend. Previous PR: #11439 (recreated to get a fresh reviewable page) Doesn't yet pass all tests, mostly ready for review ---  - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors (Will fix later)  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11556)


        Auto merge of #11556 - Manishearth:make-fetch-happen, r=jdm

Make fetch happen  Moves XHR over to the fetch backend. Previous PR: #11439 (recreated to get a fresh reviewable page) Doesn't yet pass all tests, mostly ready for review ---  - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors (Will fix later)  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11556)


        Auto merge of #11556 - Manishearth:make-fetch-happen, r=jdm

Make fetch happen  Moves XHR over to the fetch backend. Previous PR: #11439 (recreated to get a fresh reviewable page) Doesn't yet pass all tests, mostly ready for review ---  - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors (Will fix later)  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11556)

highfive assigned emilio May 26, 2016

Manishearth assigned jdm and unassigned emilio May 26, 2016

KiChjang reviewed May 26, 2016
View changes

Manishearth force-pushed the Manishearth:make-fetch-happen branch from 1e3e246 to fe71d4a May 27, 2016

Manishearth force-pushed the Manishearth:make-fetch-happen branch from fe71d4a to 090e5a6 May 27, 2016

Manishearth force-pushed the Manishearth:make-fetch-happen branch from 090e5a6 to 59978fb May 27, 2016

Manishearth force-pushed the Manishearth:make-fetch-happen branch from 59978fb to f859b94 May 27, 2016

Manishearth force-pushed the Manishearth:make-fetch-happen branch from f859b94 to da02b00 Jun 1, 2016

highfive removed the S-needs-code-changes label Jun 1, 2016

Manishearth force-pushed the Manishearth:make-fetch-happen branch from 831cbfe to c10f00f Jun 1, 2016

Manishearth changed the title ~~(WIP) Revamp fetch async code~~ (WIP) Make fetch happen Jun 1, 2016

Manishearth force-pushed the Manishearth:make-fetch-happen branch 3 times, most recently from 8788dd9 to d7a6279 Jun 1, 2016

Manishearth force-pushed the Manishearth:make-fetch-happen branch 2 times, most recently from 23f8334 to f3aeb68 Jun 1, 2016

Manishearth force-pushed the Manishearth:make-fetch-happen branch 2 times, most recently from 677522f to a9accf7 Jun 2, 2016

Manishearth added 5 commits May 26, 2016

Revamp Fetch async handling to use a fetch target and done channels

74be15f

Net side of XHR fetch integration

922c6b0

Abstractify the Listener abstractions further

25cca49

Hook XHR up to fetch

79784e7

Use RequestInit instead of LoadData. Make code look like the spec.

445601f

Manishearth force-pushed the Manishearth:make-fetch-happen branch from a9accf7 to 2586721 Jun 2, 2016

Manishearth mentioned this pull request Jun 2, 2016

Error while linking: unsupported reloc 42 against global symbol #11555

Closed

Manishearth force-pushed the Manishearth:make-fetch-happen branch from 2586721 to 4bb80b1 Jun 2, 2016

Manishearth closed this Jun 2, 2016

Manishearth mentioned this pull request Jun 2, 2016

Make fetch happen #11556

Merged

2 of 4 tasks complete

servo / servo

(WIP) Make fetch happen #11439

(WIP) Make fetch happen #11439

Manishearth commented May 26, 2016 •

edited by larsbergstrom

highfive commented May 26, 2016

highfive commented May 26, 2016

KiChjang commented May 26, 2016

This comment has been minimized.

highfive commented May 27, 2016

Manishearth commented May 27, 2016 •

edited

highfive commented May 27, 2016

jdm commented May 27, 2016

highfive commented May 27, 2016

jdm commented May 27, 2016

highfive commented May 27, 2016

bors-servo commented May 28, 2016

jdm commented May 30, 2016

Manishearth commented May 31, 2016

jdm commented May 31, 2016

Manishearth commented Jun 1, 2016

Manishearth commented Jun 2, 2016

Manishearth commented Jun 2, 2016

servo / servo

Join GitHub today

(WIP) Make fetch happen #11439

(WIP) Make fetch happen #11439

Conversation

Manishearth commented May 26, 2016 • edited by larsbergstrom

highfive commented May 26, 2016

highfive commented May 26, 2016

KiChjang commented May 26, 2016

This comment has been minimized.

KiChjang May 26, 2016

highfive commented May 27, 2016

Manishearth commented May 27, 2016 • edited

highfive commented May 27, 2016

jdm commented May 27, 2016

highfive commented May 27, 2016

jdm commented May 27, 2016

highfive commented May 27, 2016

bors-servo commented May 28, 2016

jdm commented May 30, 2016

Manishearth commented May 31, 2016

jdm commented May 31, 2016

Manishearth commented Jun 1, 2016

Manishearth commented Jun 2, 2016

Manishearth commented Jun 2, 2016

Manishearth commented May 26, 2016 •

edited by larsbergstrom

Manishearth commented May 27, 2016 •

edited