Page MenuHomePhabricator
Create Task
Maniphest T204055

Consistently represent asynchronous code execution
Closed, ResolvedPublic5 Estimated Story Points
Actions

Description

@mobrovac raised the concern that Proton mixes Promise-based and callback-based styles to represent asynchronous code execution. Both the service and the Puppeteer library are Promise-based but the queue is callback-based.

This mixing of styles has led to increased cognitive load when reviewing recent changes to the service, leading to a few avoidable delays.

@mobrovac suggests (and @pmiazga and I, at least, agree) that the queue is rewritten to be Promise-based before the service is fully deployed to production.

AC

Details

SubjectRepoBranchLines +/-
Rewrite Queue to promise-way flowmediawiki/services/chromium-rendermaster+1 K -669
Proton: Enable cancellable promisesoperations/puppetproduction+2 -1
Hygiene: Queue should know nothing about puppeteer/pdf flagsmediawiki/services/chromium-rendermaster+29 -50
Html2pdf route should return promisemediawiki/services/chromium-rendermaster+66 -42
Customize query in gerrit

Related Objects
Search...

StatusSubtypeAssignedTask
 ResolvedovasilevaT181079 [GOAL] Provide an expanded reading experience by improving the ways that users can download articles of interest for later consumption
 ResolvedNoneT181084 [EPIC] Deploy the mediawiki-services-chromium-render service (Proton)
 ResolvedBawolffT177765 Security review of mediawiki-services-chromium-render
 ResolvedphuedxT178071 [EPIC] Complete the First Deployment tutorial/checklist for the trial render service
 Resolved• mobrovacT178168 Scap3ize the deploy repository
 Resolved• mobrovacT178172 Create Puppet role and profile
 ResolvedphuedxT181118 [EPIC] Build a Chromium-based PDF renderer service
 ResolvedJdrewniakT179552 Set up Jenkins for chromium-render repository
 ResolvedphuedxT180601 Add logging to the mediawiki-services-chromium-render service
 ResolvedphuedxT180626 [Spike 8hr] How should we limit resources used by chromium render service?
 ResolvedphuedxT178278 Performance test the service
 ResolvedphuedxT178501 Limit resources used by Chromium in order to make the chromium-render service ready for production
 DeclinedNoneT178186 Update Node.js version/dependencies in package.json
 Resolved• bmansurovT176627 Trial replacing Electron with headless Chromium in the render service
 Resolved• mobrovacT180800 Update to service-template-node v0.5.3
 ResolvedovasilevaT180604 Chromium render keeps tasks in the queue even browser connection is lost
 InvalidNoneT183161 Performance test books on chromium rendering service
 ResolvedJdlrobsonT184609 [4hrs] Document the service
 ResolvedphuedxT189307 Instrument the Proton service to match mediawiki-services-electron-render
 ResolvedphuedxT181097 Update puppeteer regularly
 Resolved• holger.knustT210651 Switch all PDF render traffic to new Proton service
 ResolvedTgrT213362 Limit what URLs Proton can access
 ResolvedjijikiT226675 Undeploy electron service from WMF production
 ResolvedMSantosT226677 Expand RESTBase public PDF rendering API to support additional options supported by Proton
 Resolved• mobrovacT197862 Increase the CPU count for proton[12]00[12]
 ResolvedovasilevaT210793 Verify that PDFs generated by Proton service are correct
 ResolvedphuedxT210652 Handoff Proton service to Reading Infrastructure
 DeclinedpmiazgaT211375 Rename chromium-render gerrit project to Proton
 ResolvedDzahnT211382 Requesting access to Proton for pmiazga, bearND, Mholloway, MSantos, Tgr
 ResolvedpmiazgaT213363 Fix or clarify Proton Puppeteer sandboxing and SSL settings
 Duplicate• holger.knustT215557 Remove electron pdf endpoint backend in RESTBase
 ResolvedMSantosT213366 [2 hrs] Decide on handling system updates for Proton
 ResolvedTgrT217724 Investigate 2019-03-01 Proton incident
 ResolvedJohanT220648 Communications plan for Electron -> Proton switchover
 OpenNoneT158181 Aim for workflow equivalence for MediaWiki on desktop and mobile web
 ResolvedovasilevaT163472 [EPIC] Provide a way to download articles in PDF on the mobile website
 InvalidNoneT181513 Prepare for deploy of chromium rendering service and usage on mobile (traffic)
 Resolved• mobrovacT186748 [EPIC] New service request: chromium-render/deploy
 ResolvedphuedxT204055 Consistently represent asynchronous code execution
 ResolvedpmiazgaT209070 Allow BBPromise cancellation

Event Timeline

phuedx created this task.Sep 11 2018, 1:46 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 11 2018, 1:46 PM
phuedx added a parent task: T186748: [EPIC] New service request: chromium-render/deploy.Sep 11 2018, 1:55 PM
phuedx updated the task description. (Show Details)Sep 12 2018, 2:44 PM
• mobrovac added a project: Services (watching).Sep 20 2018, 10:33 AM
ovasileva triaged this task as High priority.Sep 20 2018, 2:41 PM
ovasileva added a project: Web-Team-Backlog.
cscott subscribed.EditedSep 20 2018, 4:16 PM

Agreed. In my experience the callback style makes it very easy to "lose" unexpected exceptions, which can be a big problem in production. Using a promise-based mechanism will facilitate debugging the inevitable problems in production.

You might consider jumping entirely to a async/await coding style---this is also Promise-based under the hood, and will interoperate w/o problem with existing code using Promises, but makes the code much more readable when chains of promises are involved.

ovasileva moved this task from Incoming to Upcoming on the Web-Team-Backlog board.Sep 20 2018, 5:30 PM
gerritbot subscribed.Sep 24 2018, 9:32 PM

Change 462579 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/services/chromium-render@master] Html2pdf route should return promise

https://gerrit.wikimedia.org/r/462579

gerritbot added a project: Patch-For-Review.Sep 24 2018, 9:32 PM
pmiazga mentioned this in rMSCR2efc4fb0b26c: Html2pdf route should return promise.Sep 24 2018, 9:57 PM
pmiazga mentioned this in rMSCRf316d481c854: Html2pdf route should return promise.Sep 24 2018, 10:09 PM
pmiazga mentioned this in rMSCR219a0a053fbb: Html2pdf route should return promise.Sep 24 2018, 10:12 PM
pmiazga mentioned this in rMSCR6908d04e93a2: Html2pdf route should return promise.Sep 25 2018, 4:21 PM
ovasileva set the point value for this task to 5.Sep 25 2018, 4:34 PM
ovasileva edited projects, added Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q1); removed Web-Team-Backlog.
pmiazga mentioned this in rMSCR80100691b6ab: Html2pdf route should return promise.Sep 25 2018, 5:42 PM
pmiazga mentioned this in rMSCRf1ca1121d387: Html2pdf route should return promise.Sep 26 2018, 1:24 PM
pmiazga mentioned this in rMSCR710cb624771c: Html2pdf route should return promise.Sep 26 2018, 1:56 PM
gerritbot added a comment.Sep 26 2018, 2:02 PM

Change 462579 merged by jenkins-bot:
[mediawiki/services/chromium-render@master] Html2pdf route should return promise

https://gerrit.wikimedia.org/r/462579

pmiazga moved this task from To Do to Doing on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q1) board.Sep 26 2018, 3:42 PM
pmiazga claimed this task.Sep 26 2018, 5:04 PM
ovasileva moved this task from Readers-Web-Kanbanana-Board-2018-19-Q1 to Readers-Web-Kanbanana-Board-2018-19-Q2 on the Web-Team-Backlog board.Oct 2 2018, 5:29 PM
ovasileva edited projects, added Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2); removed Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q1).
ovasileva moved this task from To Do to Doing on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Oct 2 2018, 5:34 PM
pmiazga mentioned this in T186748: [EPIC] New service request: chromium-render/deploy.Oct 9 2018, 5:05 PM
gerritbot added a comment.Oct 11 2018, 12:16 AM

Change 465803 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/services/chromium-render@master] Hygiene: Queue should know nothing about puppeteer/pdf flags

https://gerrit.wikimedia.org/r/465803

gerritbot added a comment.Oct 11 2018, 12:16 AM

Change 465804 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/services/chromium-render@master] Rewrite Queue to promise-way flow

https://gerrit.wikimedia.org/r/465804

pmiazga mentioned this in rMSCR8a5d54db5180: Hygiene: Queue should know nothing about puppeteer/pdf flags.Oct 11 2018, 12:24 AM
pmiazga mentioned this in rMSCR953047396a56: Rewrite Queue to promise-way flow.
pmiazga mentioned this in rMSCR2c54d7c36049: Hygiene: Queue should know nothing about puppeteer/pdf flags.Oct 11 2018, 12:31 AM
pmiazga mentioned this in rMSCR434945e3ea4e: Rewrite Queue to promise-way flow.Oct 11 2018, 12:34 AM
pmiazga moved this task from Doing to Needs Code Review on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Oct 11 2018, 12:35 AM
gerritbot added a comment.Oct 15 2018, 5:13 PM

Change 465803 merged by jenkins-bot:
[mediawiki/services/chromium-render@master] Hygiene: Queue should know nothing about puppeteer/pdf flags

https://gerrit.wikimedia.org/r/465803

Jdlrobson moved this task from Needs Code Review to Needs More Work on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Oct 16 2018, 6:13 PM
pmiazga moved this task from Needs More Work to Doing on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Oct 18 2018, 8:27 PM
pmiazga moved this task from Doing to Needs Code Review on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Oct 23 2018, 12:11 AM
pmiazga mentioned this in rMSCRd3812f53fc5e: Rewrite Queue to promise-way flow.Oct 23 2018, 12:12 AM
phuedx moved this task from Needs Code Review to Needs More Work on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Nov 1 2018, 5:52 PM

Per the latest review for rMSCRd3812f53fc5e: Rewrite Queue to promise-way flow.

• Niedzielski moved this task from Needs More Work to Doing on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Nov 7 2018, 6:12 PM
Jdlrobson added subscribers: • Pchelolo, Jdlrobson.Nov 7 2018, 6:13 PM

@pmiazga will push a new patch shortly then we plan to dedicate a 1hr code review session with myself, @mobrovac and @Pchelolo

pmiazga moved this task from Doing to Blocked on Others on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Nov 8 2018, 4:21 PM

Blocked on https://phabricator.wikimedia.org/T209070

pmiazga mentioned this in T209070: Allow BBPromise cancellation.Nov 8 2018, 4:22 PM
pmiazga added a subtask: T209070: Allow BBPromise cancellation.
pmiazga mentioned this in rMSCR2210d7a0eaa8: Rewrite Queue to promise-way flow.Nov 8 2018, 10:41 PM
pmiazga moved this task from Blocked on Others to Doing on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Nov 9 2018, 6:16 PM
pmiazga moved this task from Doing to Needs Code Review on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Nov 9 2018, 6:39 PM
pmiazga mentioned this in rMSCR6b4dca6a4c49: Rewrite Queue to promise-way flow.Nov 9 2018, 6:42 PM
• mobrovac closed subtask T209070: Allow BBPromise cancellation as Resolved.Nov 9 2018, 8:21 PM
gerritbot added a comment.Nov 9 2018, 9:47 PM

Change 472735 had a related patch set uploaded (by Mobrovac; owner: Mobrovac):
[operations/puppet@production] Proton: Enable cancellable promises

https://gerrit.wikimedia.org/r/472735

gerritbot added a comment.Nov 13 2018, 1:58 PM

Change 472735 merged by Alexandros Kosiaris:
[operations/puppet@production] Proton: Enable cancellable promises

https://gerrit.wikimedia.org/r/472735

pmiazga mentioned this in rMSCR86c7710e5b4b: Rewrite Queue to promise-way flow.Nov 14 2018, 4:50 PM
pmiazga mentioned this in rMSCRc54f71010cda: Rewrite Queue to promise-way flow.Nov 16 2018, 8:44 PM
pmiazga mentioned this in rMSCR9106aaa21d16: Rewrite Queue to promise-way flow.Nov 16 2018, 9:07 PM
pmiazga mentioned this in rMSCR6ba6069eb46e: Rewrite Queue to promise-way flow.Nov 19 2018, 3:56 PM
gerritbot added a comment.Nov 19 2018, 9:45 PM

Change 465804 merged by jenkins-bot:
[mediawiki/services/chromium-render@master] Rewrite Queue to promise-way flow

https://gerrit.wikimedia.org/r/465804

pmiazga moved this task from Needs Code Review to Ready for Signoff on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q2) board.Nov 20 2018, 4:25 PM
Jdlrobson added a comment.Nov 20 2018, 6:06 PM

Next steps: @pmiazga needs to get deployed before signing this task off. @phuedx should probably sign this off when done.

Stashbot subscribed.Nov 27 2018, 1:15 PM

Mentioned in SAL (#wikimedia-operations) [2018-11-27T13:15:13Z] <pmiazga@deploy1001> Started deploy [proton/deploy@9efc072]: Proton: Rewrite Queue to promise-way flow (T204055)

Stashbot added a comment.Nov 27 2018, 1:17 PM

Mentioned in SAL (#wikimedia-operations) [2018-11-27T13:17:56Z] <pmiazga@deploy1001> Finished deploy [proton/deploy@9efc072]: Proton: Rewrite Queue to promise-way flow (T204055) (duration: 02m 43s)

pmiazga reassigned this task from pmiazga to phuedx.Nov 27 2018, 1:24 PM
pmiazga removed a project: Patch-For-Review.

Proton deployed to production. @phuedx can you sign off?

phuedx added a comment.Nov 27 2018, 3:53 PM

👍

phuedx updated the task description. (Show Details)Nov 29 2018, 10:17 AM

Queue#push returns a promise

https://github.com/wikimedia/mediawiki-services-chromium-render/blob/6ba6069eb46e803f9249226dbca0222c34b3cac3/lib/queue.js#L132

phuedx added a comment.Nov 29 2018, 10:18 AM

Queue is rewritten to use promises internally

Done in rMSCR6ba6069eb46e: Rewrite Queue to promise-way flow.

phuedx updated the task description. (Show Details)Nov 29 2018, 10:18 AM
phuedx closed this task as Resolved.Nov 29 2018, 10:56 AM

I've done some basic tests on the service (printing various mainspace pages) and observed that it functions as expected.

Moreover, @pmiazga has been load testing the service with siege for hours and monitoring its performance and output.

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL