Pmiazga has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/392932 )
Change subject: Abort render on connection close
......................................................................
Abort render on connection close
When the client closes the connection:
- if the task is still waiting in the queue, remove it;
- if the task has already started, abort render.
Bug: T180604
Change-Id: I47f6847948ed8903c54fdaaf4fcb5ff021d46c76
---
M lib/queue.js
M package.json
M routes/html2pdf-v1.js
M test/lib/queue.js
4 files changed, 129 insertions(+), 15 deletions(-)
Approvals:
Pmiazga: Verified; Looks good to me, approved
Jdlrobson: Looks good to me, but someone else must approve
diff --git a/lib/queue.js b/lib/queue.js
index ff437ac..525f75b 100644
--- a/lib/queue.js
+++ b/lib/queue.js
@@ -2,7 +2,6 @@
const asyncQueue = require('async/queue');
const asyncTimeout = require('async/timeout');
-const uuid = require('cassandra-uuid');
// Errors used as the first argument of the callback passed to the queue
const callbackErrors = {
@@ -79,14 +78,13 @@
const queue = this._queueObject;
const timeout = this._options.queueTimeout;
- data._id = `${uuid.TimeUuid.now().toString()}|${data.uri}`;
data._timeoutID = setTimeout(() => {
queue.remove((worker) => {
- if (worker.data._id === data._id) {
+ if (worker.data.id === data.id) {
logger.log(
'warn/queue',
`Queue is still busy after waiting ` +
- `for ${timeout} secs. Data ID: ${data._id}.`
+ `for ${timeout} secs. Data ID: ${data.id}.`
);
callback(callbackErrors.queueBusy, null);
return true;
@@ -123,11 +121,12 @@
callback(callbackErrors.queueFull, null);
return;
}
+
// make sure to cancel the task if it doesn't start within a timeframe
this._setCancelTaskTimeout(data, callback);
const queueSize = this._countJobsInQueue();
this._logger.log(
- 'debug/queue', `Job ${data._id} added to the queue. Jobs waiting:
${queueSize}`
+ 'debug/queue', `Job ${data.id} added to the queue. Jobs waiting:
${queueSize}`
);
this._queueObject.push(data, callback);
}
@@ -143,21 +142,20 @@
*/
_worker(data, callback) {
this._clearCancelTaskTimeout(data);
- this._logger.log('info/queue', `Started rendering ${data._id}`);
+ this._logger.log('info/queue', `Started rendering ${data.id}`);
const timeout = this._options.executionTimeout;
const timedRender = asyncTimeout(this._render.bind(this),
timeout * 1000);
- const renderer = data._renderer;
timedRender(data, (error, pdf) => {
// error returned by async timeout
if (error && error.code === 'ETIMEDOUT') {
this._logger.log(
'error/render',
`Aborting. Render hasn't finished within ${timeout} ` +
- `seconds. Data ID: ${data._id}.`
+ `seconds. Data ID: ${data.id}.`
);
- renderer.abortRender();
+ data.renderer.abortRender();
callback(callbackErrors.renderTimeout, null);
} else {
callback(error, pdf);
@@ -171,23 +169,53 @@
* @param {Function} callback called on render success/failure
*/
_render(data, callback) {
- data._renderer
+ data.renderer
.articleToPdf(data.uri, data.format, this._puppeteerOptions,
this._pdfOptions)
.then((pdf) => {
this._logger.log(
- 'debug/queue', `Job ${data._id} rendered successfully`
+ 'debug/queue', `Job ${data.id} rendered successfully`
);
callback(null, pdf);
})
.catch((error) => {
this._logger.log('error/render', {
- msg: `Cannot convert page ${data.uri} to PDF. Error:
${error.toString()}`,
+ msg: `Data ID: ${data.id} to PDF. ${error.toString()}`,
error
});
callback(callbackErrors.renderFailed, null);
});
}
+
+ /**
+ * Abort task identified by `id`
+ * @param {string} id ID initially passed as part of data
+ * @param {Renderer} renderer instance of Renderer
+ */
+ abort(id, renderer) {
+ let taskStarted = true;
+
+ // has the task started already?
+ this._queueObject.remove((worker) => {
+ if (worker.data.id === id) {
+ this._logger.log(
+ 'debug/queue',
+ `Removing task from the queue. Data ID: ${id}.`
+ );
+ taskStarted = false;
+ return true;
+ }
+ return false;
+ });
+
+ if (taskStarted) {
+ this._logger.log(
+ 'debug/render',
+ `Aborting render. Data ID: ${id}.`
+ );
+ renderer.abortRender();
+ }
+ }
}
module.exports = {
diff --git a/package.json b/package.json
index c2dd183..9f2449e 100644
--- a/package.json
+++ b/package.json
@@ -37,13 +37,13 @@
"compression": "^1.7.1",
"domino": "^1.0.30",
"express": "^4.16.2",
+ "http-shutdown": "^1.2.0",
"js-yaml": "^3.10.0",
"preq": "^0.5.3",
"puppeteer": "^0.11.0",
"service-runner": "^2.4.2",
"swagger-router": "^0.7.1",
- "swagger-ui": "git+https://github.com/wikimedia/swagger-ui#master";,
- "http-shutdown": "^1.2.0"
+ "swagger-ui": "git+https://github.com/wikimedia/swagger-ui#master";
},
"devDependencies": {
"extend": "^3.0.1",
diff --git a/routes/html2pdf-v1.js b/routes/html2pdf-v1.js
index f1993e0..73dae5e 100644
--- a/routes/html2pdf-v1.js
+++ b/routes/html2pdf-v1.js
@@ -2,6 +2,7 @@
const { callbackErrors, Queue } = require('../lib/queue');
const sUtil = require('../lib/util');
+const uuid = require('cassandra-uuid');
const Renderer = require('../lib/renderer');
/**
@@ -27,8 +28,10 @@
}
});
+ const id = `${uuid.TimeUuid.now().toString()}|${restbaseRequest.uri}`;
const renderer = new Renderer();
app.queue.push({
+ id,
renderer,
uri: restbaseRequest.uri,
format: req.params.format
@@ -60,6 +63,15 @@
res.writeHead(200, headers);
res.end(pdf, 'binary');
}));
+
+ req.on('close', () => {
+ app.logger.log(
+ 'debug/request',
+ `Connection closed by the client. ` +
+ `Will try and cancel the task with ID ${id}.`
+ );
+ app.queue.abort(id, renderer);
+ });
});
module.exports = function(appObj) {
diff --git a/test/lib/queue.js b/test/lib/queue.js
index 3d63bad..91a1cf6 100644
--- a/test/lib/queue.js
+++ b/test/lib/queue.js
@@ -24,7 +24,7 @@
}
};
-describe('concurrency', function() {
+describe('Queue', function() {
this.timeout(5000);
it('should run only one worker at a time', function(done) {
@@ -201,4 +201,78 @@
done();
});
});
+
+ it('should remove task from queue when task is aborted', function(done) {
+ class QueueTest extends Queue {
+ _render(data, callback) {
+ // simulate render
+ setTimeout(() => {
+ callback(null, {});
+ }, data.timeout);
+ }
+ }
+ const q = new QueueTest({
+ concurrency: 1,
+ queueTimeout: 30,
+ executionTimeout: 10,
+ maxTaskCount: 10
+ }, puppeteerFlags, pdfOptions, logger);
+
+ q.push({
+ id: 1,
+ renderer,
+ timeout: 1000
+ }, (error, data) => {
+ assert.ok(error === null,
+ 'Render finished.');
+ assert.ok(q._countJobsInQueue() === 0,
+ 'Queue is empty.');
+ done();
+ });
+
+ q.push({
+ id: 2,
+ renderer,
+ timeout: 500
+ }, (error, data) => {
+ assert(false, 'Callback should never be called as the job has ' +
+ 'been removed from the queue.');
+ });
+ q.abort(2);
+ });
+
+ it('should abort render when task is aborted', function(done) {
+ class QueueTest extends Queue {
+ _render(data, callback) {
+ // simulate render
+ setTimeout(() => {
+ callback(null, {});
+ }, data.timeout);
+ }
+ }
+ const q = new QueueTest({
+ concurrency: 1,
+ queueTimeout: 30,
+ executionTimeout: 10,
+ maxTaskCount: 10
+ }, puppeteerFlags, pdfOptions, logger);
+ const renderer = {
+ abortRender: () => {
+ assert.ok(true, 'Renderer abort is called.');
+ done();
+ }
+ };
+
+ q.push({
+ id: 1,
+ renderer,
+ timeout: 5000
+ }, (error, data) => {});
+
+ // wait a little for the task to start
+ setTimeout(() => {
+ q.abort(1, renderer);
+ }, 20);
+ });
+
});
--
To view, visit https://gerrit.wikimedia.org/r/392932
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I47f6847948ed8903c54fdaaf4fcb5ff021d46c76
Gerrit-PatchSet: 4
Gerrit-Project: mediawiki/services/chromium-render
Gerrit-Branch: master
Gerrit-Owner: Bmansurov <bmansu...@wikimedia.org>
Gerrit-Reviewer: Bmansurov <bmansu...@wikimedia.org>
Gerrit-Reviewer: Jdlrobson <jrob...@wikimedia.org>
Gerrit-Reviewer: Mobrovac <mobro...@wikimedia.org>
Gerrit-Reviewer: Phuedx <samsm...@wikimedia.org>
Gerrit-Reviewer: Pmiazga <pmia...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
