Make resource requests cancelable (issue 4974)

[marcusklaas]

My attempt at tackling issue #4974. Note that even though it should be possible to cancel requests now, it is not actually used anywhere yet.

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Ms2ger (or someone else) soon.

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/4786

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[jdm]

Thanks for this PR @marcusklaas! There are two main issues with the code right now:

• the hashmap in the resource manager never has elements removed from it
• we need to ensure that we send a Done(Err) result any time a cancelled request returns early, which will allow consumers to know that there are no more messages pending.

I think we can solve both of these problems together by creating an AutoResponseCleanup value that has a destructor that:

• sends a message to the resource task to remove the appropriate value from the hashmap
• calls start_sending/sends a Done(Err) value/does nothing as appropriate (we can trigger different states based on how we progress through the response

Does that make sense?

[jdm]

Reviewed files:

• components/net/about_loader.rs @ r1
• components/net/data_loader.rs @ r1
• components/net/file_loader.rs @ r1
• components/net/http_loader.rs @ r1
• components/net/image_cache_task.rs @ r1
• components/net/resource_task.rs @ r1
• components/net_traits/lib.rs @ r1
• components/script/dom/xmlhttprequest.rs @ r1
• components/script/script_task.rs @ r1
• tests/unit/net/data_loader.rs @ r1
• tests/unit/net/resource_task.rs @ r1

---

^{components/net/file_loader.rs, line 39 [r1] (raw file):}
We should be checking for cancellation in here, too.

---

^{components/net/http_loader.rs, line 354 [r1] (raw file):}
We should be checking for cancellation in here, too.

---

^{components/net/resource_task.rs, line 251 [r1] (raw file):}
We should just silently drop this request if it's not in the hashmap.

---

^{components/net/resource_task.rs, line 290 [r1] (raw file):}
resource_sender.map

---

---

Comments from the review on Reviewable.io

[marcusklaas]

Thank you for feedback and patience Josh! I understand the issues, but it's not exactly clear to me how this AutoResponseCleanup value would work, and where it would live, specifically.

Please forgive my ignorance, but can't we just remove the relevant item from the hash map when we receive a cancel control message? This would solve the first issue, no?

[jdm]

Well, we also need to remove the item when the response is complete, regardless if it was canceled or not.

[jdm]

@marcusklaas With respect to where it would live, the struct definition and its methods can live in resource_task.rs. I envision it being used like so:

let mut response_cleanup = AutoResponseCleanup::new(...);
...
// send request
response_cleanup.note_request_sent();
// if response_cleanup is destroyed after this point via an early return, it will avoid calling start_sending
...
// read complete response
response_cleanup.note_response_complete()
// if response_cleanup is destroyed after this point, it will not send a `Done` message, since one has already been sent

The destructor would look something like:

impl Drop for AutoResponseCleanup {
    fn drop(&mut self) {
        // if start_sending has not been called {
            let tx = start_sending(...);
            tx.send(Done(Err));
        } else if // response incomplete {
            self.stored_progress_chan.send(Done(Err));
        }
        self.resource_task.send(Cleanup(self.id));
    }
}

Is that more clear?

[marcusklaas]

Apologies for the delay in addressing these issues. I very much intend to finish this in the next couple of days.

[jdm]

Not a problem!

[jdm]

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

We still need to add the auto removal and Done message sending as described previously.

---

Reviewed files:

• components/net/file_loader.rs @ r2
• components/net/http_loader.rs @ r2
• components/net/resource_task.rs @ r2

---

^{components/net/file_loader.rs, line 36 [r2] (raw file):}
I would prefer an enum value for clarity.

---

^{components/net/http_loader.rs, line 363 [r2] (raw file):}
Enum for clarity.

---

^{components/net/resource_task.rs, line 251 [r2] (raw file):}
Use if let instead of map when the return value isn't being used.

---

Comments from the review on Reviewable.io

[marcusklaas]

This has bitrotten terribly and I don't think I can bring this to a proper end any more.. Shall I close this PR?

Sorry for taking up your time, @jdm! 😟

[jdm]

Not a problem! This helped expose complexities in the design, so it served a useful purpose :)