Page MenuHomePhabricator

Chromium render keeps tasks in the queue even browser connection is lost
Closed, ResolvedPublic3 Story Points

Description

While testing the concurrency I found out that even I stop siege tests queue is still full and processing all stored requests.

Steps to reproduce:
  • using browser/curl/siege fill the job with long waiting tasks till you get "queue is full error"
  • stop those requests (close siege/browser)
  • try to send one more request to the queue
Expected result:

Queue gets drained, all waiting/running jobs gets terminated as open sockets became closed

Current behaviour:

The queue is still processing jobs even connection is closed. Adding new jobs fails with an error before all queued items gets rendered.

We should cancel running jobs if user closes the connection,

Details

Related Gerrit Patches:
mediawiki/services/chromium-render : masterAbort render on connection close

Event Timeline

pmiazga created this task.Nov 15 2017, 3:00 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 15 2017, 3:00 PM
pmiazga added a subscriber: mobrovac.EditedNov 15 2017, 3:03 PM

@mobrovac Do you know how we can solve this problem? In pure nodejs app we could listen to close or aborted events, but I have no idea how we can tackle that with our ServiceRunner. Can we access socket somehow via req variable here https://github.com/wikimedia/mediawiki-services-chromium-render/blob/master/routes/html2pdf-v1.js#L19 ?

So this sounds like the browser instance hasn't been terminated when expected. I haven't yet had time to really look into this, but it looks like there have been some changes in the browser.close() API which might help with this, so updating to puppeteer v0.13.0 might help (and by looking at the list of changes since v0.11.0 would actually be advisable).

In general with fast-paced-developing software like puppeteer it's always a good idea to check if updates and/or issues might help with the problems that are being witnessed.

I'm not talking about puppeteer browser instance. I meant situation when user closes the tab.
An example situation: user clicks a link to render PDF, waits like 10-15 seconds and decides to close the tab and try again later. The job will stay in the queue, job will be picked up from the queue and rendering will start to happen even there is no open connection between user browser and the node service.

Oh right, right. In that case, the req object fires a close event which can be used in the service. So, something like:

req.on('close', () =>{
  // remove from queue
  // close browser window
});

in the route and/or logic should suffice.

(My comment about updating to the newest version of puppeteer still stands.)

ovasileva triaged this task as High priority.Nov 20 2017, 6:11 PM

Updating puppeteer has been moved to a separate task: T181097: Update puppeteer regularly.

bmansurov moved this task from To Do to Doing on the Readers-Web-Kanbanana-Board-Old board.
ovasileva set the point value for this task to 3.Nov 22 2017, 5:45 PM

Change 392932 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/services/chromium-render@master] Abort render on connection close

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

Change 392932 merged by Pmiazga:
[mediawiki/services/chromium-render@master] Abort render on connection close

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

Jdlrobson added a subscriber: Jdlrobson.

This is skipping QA. It was done as part of code review (Piotr used siege stress test tool)

bmansurov removed bmansurov as the assignee of this task.Nov 28 2017, 11:27 PM

Task is ready, tested locally, can be signed-off

Jdlrobson added a subscriber: ovasileva.

@ovasileva this has been reviewing technically and in our opinion can be resolved. During standup we didn't want to resolve it to let you take a look and see if there's anything you have to add.

ovasileva closed this task as Resolved.Nov 30 2017, 8:01 PM

looks good to me - resolving!