Consistently represent asynchronous code execution
Closed, ResolvedPublic5 Story Points

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

  • Queue is rewritten to use promises internally
  • Queue#push returns a promise

Related Objects

StatusAssignedTask
StalledNone
Openphuedx
OpenBawolff
Resolvedphuedx
Resolvedmobrovac
Resolvedmobrovac
Resolvedphuedx
ResolvedJdrewniak
Resolvedphuedx
Resolvedphuedx
Resolvedphuedx
Resolvedphuedx
DeclinedNone
Resolved bmansurov
Resolvedmobrovac
Resolvedovasileva
InvalidNone
ResolvedJdlrobson
Resolvedphuedx
Resolvedphuedx
OpenNone
OpenNone
OpenNone
Openpmiazga
Openpmiazga
ResolvedDzahn
OpenNone
StalledNone
Resolvedmobrovac
Resolvedphuedx
Resolvedpmiazga
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 updated the task description. (Show Details)Sep 12 2018, 2:44 PM
ovasileva triaged this task as High priority.Sep 20 2018, 2:41 PM
ovasileva added a project: Readers-Web-Backlog.
cscott added a subscriber: cscott.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.

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

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

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

pmiazga claimed this task.Sep 26 2018, 5:04 PM

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

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

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

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

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

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

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

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

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

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

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)

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.Tue, Nov 27, 1:24 PM
pmiazga removed a project: Patch-For-Review.

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

Queue is rewritten to use promises internally

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

phuedx updated the task description. (Show Details)Thu, Nov 29, 10:18 AM
phuedx closed this task as Resolved.Thu, Nov 29, 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.