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

PDF.js should abort fetch requests during teardown #9698

Closed
johnhok opened this issue May 1, 2018 · 3 comments · Fixed by #9701
Closed

PDF.js should abort fetch requests during teardown #9698

johnhok opened this issue May 1, 2018 · 3 comments · Fixed by #9701
Labels

Comments

@johnhok
Copy link

johnhok commented May 1, 2018

Steps to reproduce the problem:

  1. Fetch a document with streaming enabled.
  2. Destroy the document loading task
  3. Observe that the fetch request is not aborted.

What is the expected behavior?
Fetch request should be aborted when cancel is called on PDFFetchStreamReader and PDFFetchStreamRangeReader.

Context
It seems when fetch was implemented (#6126 (comment)) abort on fetch was still in flux.

It seems now that has settled and chromium has also implemented aborting fetch requests in 66 (https://bugs.chromium.org/p/chromium/issues/detail?id=750599)

@mukulmishra18
Copy link
Contributor

mukulmishra18 commented May 2, 2018

Sounds reasonable to me. I think it is good idea to abort fetch in PDFFetchStream when document destroy.

I think all we need is to have a property something like this.controller = new AbortController() in PDFFetchStreamReader and PDFFetchStreamRangeReader and pass it to the fetch(url, { this.controller.signal, ... }) and call this.controller.abort() in cancel method.

@yurydelendik @Rob--W What you think about that?

@Rob--W
Copy link
Member

Rob--W commented May 2, 2018

Sounds good to me. Make sure to use feature detection before using AbortController.

@mukulmishra18
Copy link
Contributor

Make sure to use feature detection before using AbortController.

Okay, sure. I will try to create a PR for this tonight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants