Cancelable network requests!

[wafflespeanut]

fixes #4974

[wafflespeanut]

This is incomplete! Don't review it yet!

[jdm]

Curious time to open a pull request, in that case :)

[wafflespeanut]

@jdm Though I've stolen some of the ideas from #5826, it's still incomplete and the build doesn't succeed (so, please ignore the errors if you find any). I have a few questions...

• I'm not sure I can find all the key points where we should check for cancellation (for now, I've added those in loading/handling loops - places which I thought might need this, but I'm not sure)
• We cannot have a Sender in ControlMsg because it doesn't impl serialization. We could make use of ipc_channel, but it doesn't return a proper error (see below)

---

Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

^{components/net/resource_task.rs, line 183 [r1] (raw file):}
This is plain wrong! (ignore it, I somehow missed this). We shouldn't remove it, as we'll then index the wrong sender in the future. We could go for an Option<Sender...> (as I've said below)

---

^{components/net/resource_task.rs, line 208 [r1] (raw file):}
This is where IpcReceiver doesn't help (if we ever plan to go for it). We can't differentiate between both the cases represented by the enum. I thought of routing the IpcReceiver into a Receiver, but the method consumes the old IpcReceiver and so, we can't possibly have it here. Having it outside appears rather ugly, and I'm also not sure whether that method would be efficient.

---

^{components/net/resource_task.rs, line 224 [r1] (raw file):}
... could be Vec<Option<Sender<CancelLoad>>> so that we can mark the senders as None once we send the cancel request.

---

Comments from the review on Reviewable.io

[wafflespeanut]

@jdm Indeed. Rust keeps me awake during my weekends :)

[wafflespeanut]

Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

^{components/net/data_loader.rs, line 26 [r1] (raw file):}
Oh, and I've left this, because I can't see where I should be checking for cancel requests here :/

---

Comments from the review on Reviewable.io

[jdm]

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

---

^{components/net/data_loader.rs, line 26 [r1] (raw file):}
I would check right before sending the actual data.

---

^{components/net/file_loader.rs, line 65 [r1] (raw file):}
Careful - when we detect a cancelled channel, we need to make sure that we have sent the initial start message and a Done(Err(Canceled)) final message before returning. We should try creating a stack guard with a destructor that does this for us, if possible.

---

^{components/net/resource_task.rs, line 169 [r1] (raw file):}
Let's call this id_sender.

---

^{components/net/resource_task.rs, line 183 [r1] (raw file):}
Removing is good, otherwise we'll end up with a monotonically-increasing vector over the lifetime of the browser.

---

^{components/net/resource_task.rs, line 208 [r1] (raw file):}
This shouldn't need to be IpcReceiver, since it's only used internally in the same process.

---

^{components/net/resource_task.rs, line 224 [r1] (raw file):}
We should store a HashMap<ResourceId, Sender<CancelLoad>> instead, to allow for removal.

---

^{components/net/resource_task.rs, line 260 [r1] (raw file):}
Let's call this id_sender instead.

---

^{components/net/resource_task.rs, line 274 [r1] (raw file):}
This should ignore the error with let _ =.

---

^{components/net_traits/lib.rs, line 230 [r1] (raw file):}
This doesn't build, right? Doesn't this need to be an IpcSender?

---

Comments from the review on Reviewable.io

[bors-servo]

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

[wafflespeanut]

@jdm We still have some trouble with the setup. Firstly, the CancellationListener gets consumed along the way, which is why I thought of going for an Option<&Receiver<CancelLoad>> (since we can send the reference around anywhere). But, it demands a lifetime, which conflicts with that of the box. Any ideas how I can proceed further? Is CancelLoad fine, or are there any other ways I can send a cancel message?

(also, have a look at my current changes)

[jdm]

What problem are you trying to solve, exactly? The CancellationListener is different Receiver value, and the item that gets stored in the HashMap is the corresponding Sender, right? The code looks to me like everything should just work if we return cancel_receiver inside the map call, rather than &cancel_receiver.

[wafflespeanut]

This isn't done yet! I've just rebased and updated the tests (unit tests are on the way)...

[jdm]

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

---

Reviewed 11 of 11 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

^{components/net/resource_task.rs, line 191 [r2] (raw file):}
nit: new line here.

---

^{tests/unit/net/data_loader.rs, line 25 [r2] (raw file):}
I don't think this comment adds anything.

---

Comments from the review on Reviewable.io

[wafflespeanut]

@jdm I think this would work, but I'm not sure about the readability. Feel free to pinch me :)

---

Review status: 7 of 11 files reviewed at latest revision, 2 unresolved discussions.

---

^{components/net/resource_task.rs, line 311 [r13] (raw file):}
This is where I'm concerned about the readability and elegance. Is this okay?

---

^{components/net/resource_task.rs, line 339 [r13] (raw file):}
Also this is what I can think of, but it makes the resource manager wait till it gets the message when it removes the ResourceId from the hashmap. Is it the right way?

---

Comments from the review on Reviewable.io

[jdm]

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

---

Reviewed 4 of 4 files at r13.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

^{components/net/resource_task.rs, line 238 [r13] (raw file):}
Let's make this a destructor instead, so it's impossible to leak these resources.

---

^{components/net/resource_task.rs, line 339 [r13] (raw file):}
This will make the resource task block until the complete response is received, which is not what we want :) Let's add a new message to ControlMsg and have CancellationListener store a sender to communicate with the main event loop of the resource task.

---

Comments from the review on Reviewable.io

[jdm]

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

---

Reviewed 5 of 5 files at r14.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

^{components/net/resource_task.rs, line 208 [r14] (raw file):}
Let's make this:

struct CancellableResource {
    receiver: Receiver<CancelLoad>,
    resource_id: ResourceId,
    resource_task: ResourceTask,
}

and just have a single Option<CancellableResource> in CancellationListener.

---

^{components/net/resource_task.rs, line 249 [r14] (raw file):}
"Ensure the resource manager stops tracking this request now that it's terminated."

---

Comments from the review on Reviewable.io

[wafflespeanut]

Your suggestion looks really elegant! Thanks! :)

---

Review status: 9 of 11 files reviewed at latest revision, 1 unresolved discussion.

---

^{components/net/resource_task.rs, line 202 [r15] (raw file):}
I think we don't need a struct just for sending a cancellation message. We can achieve the same with a tuple, because we're creating them as cancel senders & receivers anyway. What do you say?

---

Comments from the review on Reviewable.io

[jdm]

@bors-servo: r+

---

Reviewed 2 of 2 files at r15.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[bors-servo]

📌 Commit c4cf72d has been approved by jdm

[bors-servo]

⌛ Testing commit c4cf72d with merge 4848e37...

[bors-servo]

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt