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

Implement fetch abortion #18671

Closed
nox opened this issue Sep 29, 2017 · 5 comments
Closed

Implement fetch abortion #18671

nox opened this issue Sep 29, 2017 · 5 comments
Labels

Comments

@nox
Copy link
Member

@nox nox commented Sep 29, 2017

There is currently no way to cancel an ongoing fetch operation. That's fine for small images and favicons, but that will not scale for media.

@nox nox added the A-network label Sep 29, 2017
@jdm
Copy link
Member

@jdm jdm commented Sep 29, 2017

We supported this for the non-fetch network stack, so we could look at reusing that code. #7844 and #8523 were the original implementation.

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Oct 14, 2017

So, to sum up the two previous PRs, we have a ResourceId struct that uniquely identifies each "load message". We also have a cancel_load_map, which is essentially a map from ResourceId to a channel that can be used to cancel the ongoing fetch.

The whole flow of cancelling goes like this:

  1. Resource thread receives a Load message containing an optional id_sender.
  2. When processing the load, we create a new ResourceId and send it back to whoever initiated the Load message, along with creating a new cancellation channel, putting the sender into the cancel_load_map keyed by the ResourceId. Then, we create a new CancellableResource, containing the receiver of the cancellation channel and the ResourceId.
  3. We wrap the CancellationResource created in the previous step with a CancellationListener.
  4. At each checkpoint of the load algorithm, we check if we received a cancellation message via the CancellationListener, and if so, we abort the load immediately, which also causes the listener to remove the corresponding ResourceId from cancel_load_map.

Going forward, I think we should make it so that cancel_load_map becomes fetch group, and make it belong to a global in the script thread. A fetch group's fetch in the spec would simply be an IPC sender to the ongoing fetch algorithm, and the corresponding IPC receiver (i.e. the cancellation receiver) is sent along with the Fetch message to the resource thread.

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Oct 25, 2017

Related: #14489

@jdm
Copy link
Member

@jdm jdm commented Nov 8, 2017

Rather than have cancel_load_map be treated as the fetch group, I think it would make more sense to have the Load message also contain an optional existing ResourceId that the fetch is associated with. When we create the new ResourceId, we would add a new entry to a fetch_group hashmap from ResourceId -> [ResourceId], and when cancelling a fetch we would check this hashmap and cancel any ResourceIds that are associated with the cancelled id. This would allow us to associate subresources with an overall document load, for instance.

@jdm
Copy link
Member

@jdm jdm commented Nov 21, 2017

This was fixed by #19274.

@jdm jdm closed this Nov 21, 2017
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.

None yet
3 participants
You can’t perform that action at this time.