[api-minor] Adds controlled destruction of the worker. by yurydelendik · Pull Request #6546 · mozilla/pdf.js

yurydelendik · 2015-10-21T02:01:03Z

Introduces PDFDocumentLoadingTask destroy() method to initiate tear down ~~and onDestroyed callback to wait on its completion~~.

Attempt to address intermittent failures during FF testing, e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1016737

That is also an alternative solution for #5629

Snuffleupagus · 2015-10-21T10:57:59Z

src/display/api.js

How about also including a warn(...) below to notify users of the deprecation (since it's not unlikely that people won't bother reading comments such as this)?

Snuffleupagus · 2015-10-21T11:08:55Z

/botio test

pdfjsbot · 2015-10-21T11:08:55Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/34925907f0c548c/output.txt

pdfjsbot · 2015-10-21T11:08:55Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/cc367b486b03a90/output.txt

pdfjsbot · 2015-10-21T11:27:06Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/cc367b486b03a90/output.txt

Total script time: 18.17 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2015-10-21T11:28:55Z

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/34925907f0c548c/output.txt

Total script time: 19.99 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://107.21.233.14:8877/34925907f0c548c/reftest-analyzer.html#web=eq.log

yurydelendik · 2015-10-21T13:36:15Z

(We need to start 1.2 version for this one)

timvandermeij · 2015-10-22T12:37:59Z

There are some merge conflicts for this patch. I'm also going to try to list all issues and PRs that this patch resolves or supersedes (some of them are closed already, but I'm listing them anyway for completeness):

Issues: Unable to cancel a transfer #3485, calling PDFView.open while PDF is loading causes both documents to be rendered #4774, Document loads should be sequential, they currently are not #5228, Exceptions when load a document while loading document #6523, Canceling getDocument? #5269, Add PDFJS.abortGetDocument function to enable cancelling of calls to PDFJS.getDocument #5234
PRs: Ensure transport is destroyed if the document fails to load #5572, [api-minor] Add a |cancel| method to PDFDocumentLoadingTask #5629

brendandahl · 2015-10-22T23:22:32Z

As discussed on IRC: changing WorkerThread name and making destroy() return a promise. Then r+

brendandahl · 2015-10-22T23:22:46Z

test/unit/api_spec.js

Should this be commented out?

yurydelendik · 2015-10-23T13:55:04Z

/botio test

pdfjsbot · 2015-10-23T13:55:04Z

From: Bot.io (Linux)

Received

Command cmd_test from @yurydelendik received. Current queue size: 0

Live output at: http://107.21.233.14:8877/0965ea6f5fdfa55/output.txt

Also renames PDFPageProxy.destroy method to cleanup.

pdfjsbot · 2015-10-23T14:14:51Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/0965ea6f5fdfa55/output.txt

Total script time: 19.78 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

yurydelendik · 2015-10-23T15:06:02Z

/botio-windows test

pdfjsbot · 2015-10-23T15:06:03Z

From: Bot.io (Windows)

Received

Command cmd_test from @yurydelendik received. Current queue size: 1

Live output at: http://107.22.172.223:8877/b5c95c1c044ab64/output.txt

pdfjsbot · 2015-10-23T15:41:53Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/b5c95c1c044ab64/output.txt

Total script time: 18.80 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

[api-minor] Adds controlled destruction of the worker.

yurydelendik · 2015-10-24T12:35:21Z

Fixes #5278

timvandermeij · 2015-10-24T12:36:06Z

Really nice work!

Snuffleupagus reviewed Oct 21, 2015
View reviewed changes

Snuffleupagus added the core label Oct 21, 2015

yurydelendik force-pushed the destroy branch 2 times, most recently from d67f6ec to 61e7bde Compare October 21, 2015 13:34

yurydelendik changed the title ~~Adds controlled destruction of the worker.~~ [api-minor] Adds controlled destruction of the worker. Oct 21, 2015

yurydelendik assigned brendandahl Oct 21, 2015

yurydelendik force-pushed the destroy branch 9 times, most recently from 1a33bc6 to ed9b40c Compare October 21, 2015 21:34

yurydelendik force-pushed the destroy branch from ed9b40c to 0b3b48f Compare October 22, 2015 21:57

brendandahl reviewed Oct 22, 2015
View reviewed changes

test/unit/api_spec.js Outdated

Copy link

Contributor

brendandahl Oct 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be commented out?

yurydelendik force-pushed the destroy branch from 0b3b48f to 96a3efc Compare October 23, 2015 13:53

Adds destroy method to the document loading task.

59c13b3

Also renames PDFPageProxy.destroy method to cleanup.

yurydelendik force-pushed the destroy branch from 96a3efc to 3e3aa55 Compare October 23, 2015 13:57

yurydelendik mentioned this pull request Oct 23, 2015

Update version number for minor API changes. #6555

Merged

yurydelendik added 3 commits October 23, 2015 09:06

Adds thread abort capabilities.

58c3ea0

Adds deprecation warning for the API calls.

5135aa9

Fixes PDFViewerApplication.open/close methods signature.

62afa9f

yurydelendik force-pushed the destroy branch from 3e3aa55 to 62afa9f Compare October 23, 2015 14:06

brendandahl added a commit that referenced this pull request Oct 23, 2015

Merge pull request #6546 from yurydelendik/destroy

2a5616c

[api-minor] Adds controlled destruction of the worker.

brendandahl merged commit 2a5616c into mozilla:master Oct 23, 2015

Snuffleupagus mentioned this pull request Oct 23, 2015

Prevent TypeError: page is undefined when the document has been destroyed (PR 6546 follow-up) #6560

Merged

This was referenced Oct 23, 2015

Unable to cancel a transfer #3485

Closed

Document loads should be sequential, they currently are not #5228

Closed

Snuffleupagus mentioned this pull request Oct 24, 2015

Uncaught TypeError: Cannot read property 'cleanup' of null #6440

Closed

This was referenced Oct 24, 2015

Would like to fix bug https://github.com/mozilla/pdf.js/issues/6549 in version PDF.js 1.1.366 or 1.1.469 #6561

Closed

Ensure transport is destroyed if the document fails to load #5572

Closed

yurydelendik mentioned this pull request Oct 24, 2015

Ensure PDFView.close() really closes and releases all resource #5278

Closed

yurydelendik deleted the destroy branch January 28, 2016 21:03

Snuffleupagus mentioned this pull request Jul 2, 2016

Remove handling of fallback arguments from PDFViewerApplication.open #7466

Merged

zmagyar mentioned this pull request Feb 13, 2020

loadingTask.destroy() throws worker exception #11595

Closed

Conversation

yurydelendik commented Oct 21, 2015

Uh oh!

Snuffleupagus Oct 21, 2015

Choose a reason for hiding this comment

Uh oh!

Snuffleupagus commented Oct 21, 2015

Uh oh!

pdfjsbot commented Oct 21, 2015

From: Bot.io (Linux)

Received

Uh oh!

pdfjsbot commented Oct 21, 2015

From: Bot.io (Windows)

Received

Uh oh!

pdfjsbot commented Oct 21, 2015

From: Bot.io (Windows)

Success

Uh oh!

pdfjsbot commented Oct 21, 2015

From: Bot.io (Linux)

Failed

Uh oh!

yurydelendik commented Oct 21, 2015

Uh oh!

timvandermeij commented Oct 22, 2015

Uh oh!

brendandahl commented Oct 22, 2015

Uh oh!

brendandahl Oct 22, 2015

Choose a reason for hiding this comment

Uh oh!

yurydelendik commented Oct 23, 2015

Uh oh!

pdfjsbot commented Oct 23, 2015

From: Bot.io (Linux)

Received

Uh oh!

pdfjsbot commented Oct 23, 2015

From: Bot.io (Linux)

Success

Uh oh!

yurydelendik commented Oct 23, 2015

Uh oh!

pdfjsbot commented Oct 23, 2015

From: Bot.io (Windows)

Received

Uh oh!

pdfjsbot commented Oct 23, 2015

From: Bot.io (Windows)

Success

Uh oh!

yurydelendik commented Oct 24, 2015

Uh oh!

timvandermeij commented Oct 24, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants