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

Add PDFJS.abortGetDocument function to enable cancelling of calls to PDFJS.getDocument #5234

Closed
wants to merge 4 commits into from
Closed

Conversation

Snuffleupagus
Copy link
Collaborator

This PR contains a tentative patch to fix #3485 (and should also fix #5228, #4774).

I was slightly unsure of the best way to handle the rejection of workerReadyCapability, since it's not easily available outside of getDocument and I didn't just want to expose it in PDFJS.
A caveat with this patch is that it's only possible to abort the current getDocument, so if you have multiple ones active simultaneously this isn't going to work. Since this implementation should work fine when used in the generic viewer (e.g. for #5228), I don't know if it's really worth complicating the solution too much.

One other thing to note here: abortGetDocument really only makes sense when getDocument isn't resolved/rejected yet, since otherwise just calling destroy is enough.

When I started looking at this, I realized that our current exception handling code wasn't really working as intended (rejection with multiple arguments). The first commit fixes this, and I'd be happy to extract that into a separate PR if that's preferred. Submitted as a separate PR.

@yurydelendik Please let me know if this PR heading in the right direction, or if you'd prefer a different/modified solution!

@CodingFabian
Copy link
Contributor

this might also fix #4774 - a colleague was able to reproduce it, if you make a preview we can try to verify.

@Snuffleupagus
Copy link
Collaborator Author

this might also fix #4774 - a colleague was able to reproduce it, if you make a preview we can try to verify.

Given that this looks exactly like #5228, it should fix that one too.

/botio-windows preview

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.22.172.223:8877/33065e59e9a7697/output.txt

Currently when an exception is thrown, we try to reject `workerReadyCapability` with multiple arguments in src/core/api.js. This obviously doesn't work, hence this patch changes that to instead reject with the exception object as is.
In src/core/worker.js the exception is currently (unncessarily) wrapped in an object, so this patch also simplifies that to directly send the exception object instead.
…itsForPromiseResolved`and add a `waitsForPromiseRejected` function
@yurydelendik
Copy link
Contributor

Will block by #5277 and #5278 for now.

@yurydelendik yurydelendik added this to the 2014 Q4 milestone Sep 8, 2014
@CodingFabian
Copy link
Contributor

#5275 was merged, you can rebase this now.

@yurydelendik
Copy link
Contributor

Blocked by #5554

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

Successfully merging this pull request may close these issues.

Document loads should be sequential, they currently are not Unable to cancel a transfer
4 participants