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

PerformanceResourceTiming: responseEnd #21263

Closed
avadacatavra opened this issue Jul 27, 2018 · 12 comments
Closed

PerformanceResourceTiming: responseEnd #21263

avadacatavra opened this issue Jul 27, 2018 · 12 comments

Comments

@avadacatavra
Copy link
Contributor

@avadacatavra avadacatavra commented Jul 27, 2018

On getting, the responseEnd attribute MUST return as follows:

  1. The time immediately after the user agent receives the last byte of the response or immediately before the transport connection is closed, whichever comes first. The resource here can be received either from relevant application caches, local resources, or from the server.
  2. The time immediately before the user agent aborts the fetch due to a network error.

Spec: https://w3c.github.io/resource-timing/#dom-performanceresourcetiming-responseend

@jdm
Copy link
Member

@jdm jdm commented Feb 3, 2019

.and_then(move |res_body| {
let mut body = res_body.lock().unwrap();
let completed_body = match *body {
ResponseBody::Receiving(ref mut body) => mem::replace(body, vec![]),
_ => vec![],
};
*body = ResponseBody::Done(completed_body);
let _ = done_sender2.send(Data::Done);
future::ok(())
})
.map_err(move |_| {
let mut body = res_body2.lock().unwrap();
let completed_body = match *body {
ResponseBody::Receiving(ref mut body) => mem::replace(body, vec![]),
_ => vec![],
};
*body = ResponseBody::Done(completed_body);
let _ = done_sender3.send(Data::Done);
}),
are two blocks of code that would need to set this attribute in the ResourceFetchTiming structure.

@vn-ki
Copy link
Contributor

@vn-ki vn-ki commented Feb 4, 2019

@jdm Which file(s) should I be looking at for this issue?

I would assume the above linked files have to be changed, but I docs.servo.org/servo is crashing on me and I can't find ResourceFetchTiming structure.

Is there any other way to access the docs?

@CYBAI
Copy link
Collaborator

@CYBAI CYBAI commented Feb 4, 2019

Is there any other way to access the docs?

https://doc.servo.org/servo/index.html

@jdm
Copy link
Member

@jdm jdm commented Feb 4, 2019

You could try running cargo doc --open and see if that makes a difference.

@jiachengpizza
Copy link

@jiachengpizza jiachengpizza commented Feb 20, 2019

Hi @jdm ,
I'm learning Rust and servo code starting from this issue. I think the responseEnd timing value should be set after Line 1290 and Line 1300, after ResponseBody::Done(). Is that right?
Thanks.

@jdm
Copy link
Member

@jdm jdm commented Feb 27, 2019

Sorry for the delay; I was on vacation. Yes, that sounds right to me.

@tdelacour
Copy link
Contributor

@tdelacour tdelacour commented Apr 2, 2019

@highfive assign me

@highfive highfive added the C-assigned label Apr 2, 2019
@highfive
Copy link

@highfive highfive commented Apr 2, 2019

Hey @tdelacour! Thanks for your interest in working on this issue. It's now assigned to you!

@tdelacour
Copy link
Contributor

@tdelacour tdelacour commented Apr 5, 2019

Hi @jdm (or whoever can help!) I've added a couple of statements inside the spawn() closure in http_loader.rs and it now looks like this:

HANDLE.lock().unwrap().spawn(
        res.into_body()
            .map_err(|_| ())
            .fold(res_body, move |res_body, chunk| {
                if cancellation_listener.lock().unwrap().cancelled() {
                    *res_body.lock().unwrap() = ResponseBody::Done(vec![]);
                    let _ = done_sender.send(Data::Cancelled);
                    return future::failed(());
                }
                if let ResponseBody::Receiving(ref mut body) = *res_body.lock().unwrap() {
                    let bytes = chunk.into_bytes();
                    body.extend_from_slice(&*bytes);
                    let _ = done_sender.send(Data::Payload(bytes.to_vec()));
                }
                future::ok(res_body)
            })
            .and_then(move |res_body| {
                let mut body = res_body.lock().unwrap();
                let completed_body = match *body {
                    ResponseBody::Receiving(ref mut body) => mem::replace(body, vec![]),
                    _ => vec![],
                };
                *body = ResponseBody::Done(completed_body);
                context // <-------------- ADDED
                    .timing
                    .lock()
                    .unwrap()
                    .set_attribute(ResourceAttribute::ResponseEnd);
                let _ = done_sender2.send(Data::Done);
                future::ok(())
            })
            .map_err(move |_| {
                let mut body = res_body2.lock().unwrap();
                let completed_body = match *body {
                    ResponseBody::Receiving(ref mut body) => mem::replace(body, vec![]),
                    _ => vec![],
                };
                *body = ResponseBody::Done(completed_body);
                context // <-------------- ADDED
                    .timing
                    .lock()
                    .unwrap()
                    .set_attribute(ResourceAttribute::ResponseEnd);
                let _ = done_sender3.send(Data::Done);
            }),
    );

But this causes a compiler error:

error[E0277]: `(dyn embedder_traits::EventLoopWaker + 'static)` cannot be shared between threads safely
    --> components/net/http_loader.rs:1278:28
     |
1278 |     HANDLE.lock().unwrap().spawn(
     |                            ^^^^^ `(dyn embedder_traits::EventLoopWaker + 'static)` cannot be shared between threads safely
     |

I think I understand, broadly, what the issue is: I'm referencing context within a new thread, but context is not threadsafe. I was able to get past the compiler by making mutable clones of the ResourceFetchTiming struct inside context and using those inside the closure, but that doesn't seem like it would accomplish... anything (the updated timing structs wouldn't be accessible anywhere outside of the closure in the spawned thread, right?)

As you can probably tell, I'm very new to Rust. I'm reading up on closures, moves, concurrency, send / sync, etc in the Rust book, but I could definitely use some more targeted help 😬

@jdm
Copy link
Member

@jdm jdm commented Apr 5, 2019

If you make a clone of context.timing outside of the closure and refer to that clone inside of it, you should be able to achieve your goal. That avoids sharing the whole context, and the timing information is already stored in an Arc wrapper (I believe), so cloning it does not actually duplicate it.

@tdelacour
Copy link
Contributor

@tdelacour tdelacour commented Apr 5, 2019

amazing, thank you!

@tdelacour tdelacour mentioned this issue Apr 8, 2019
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Apr 23, 2019
Add PerformanceResourceTiming: ResponseEnd

<!-- Please describe your changes on the following line: -->
1. Added `ResponseEnd` to `ResourceAttribute` enum in `net_traits` and added it to the `set_attribute` function on `ResourceFetchTiming`
2. Added `response_end` field to `performanceresourcetiming.rs`
3. In `http_loader.rs`, set ResponseEnd after response body read is complete, or before return due to network error.

I could use a little guidance on testing. After building and running `wpt` tests, I noticed that some tests now "Pass" when they were expected to "Fail". As per the wiki instructions, I've removed those expectations from the `metadata`.

I noticed that there are a handful of other "failing" test expectations associated with `responseEnd`, but those still do not pass. I looked through some similar PRs (`connectEnd`, `redirectStart`, etc) and saw that they also still have a few failing test expectations here and there. Does that mean this is OK for now? How can I better understand which tests we expect to resolve for a given issue? Thanks!

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #21263 (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23178)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Apr 23, 2019
Add PerformanceResourceTiming: ResponseEnd

<!-- Please describe your changes on the following line: -->
1. Added `ResponseEnd` to `ResourceAttribute` enum in `net_traits` and added it to the `set_attribute` function on `ResourceFetchTiming`
2. Added `response_end` field to `performanceresourcetiming.rs`
3. In `http_loader.rs`, set ResponseEnd after response body read is complete, or before return due to network error.

I could use a little guidance on testing. After building and running `wpt` tests, I noticed that some tests now "Pass" when they were expected to "Fail". As per the wiki instructions, I've removed those expectations from the `metadata`.

I noticed that there are a handful of other "failing" test expectations associated with `responseEnd`, but those still do not pass. I looked through some similar PRs (`connectEnd`, `redirectStart`, etc) and saw that they also still have a few failing test expectations here and there. Does that mean this is OK for now? How can I better understand which tests we expect to resolve for a given issue? Thanks!

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #21263 (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23178)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

7 participants
You can’t perform that action at this time.