jenkins-bot has submitted this change and it was merged.
Change subject: T115327: Log errors passed along in express
......................................................................
T115327: Log errors passed along in express
* Use our logger on the chained errors from helpers, like the
body-parser.
* Also, prefer res.send when passing data.
* The content-type changes are because using sendResponse reorders that
string.
Change-Id: Ibc870e15c58b45f2cf9428d8d5541480c8b8e469
---
M lib/api/ParsoidService.js
M lib/api/apiUtils.js
M lib/api/routes.js
M lib/wt2html/DOMPostProcessor.js
M npm-shrinkwrap.json
M package.json
M tests/mocha/api.js
7 files changed, 73 insertions(+), 40 deletions(-)
Approvals:
Subramanya Sastry: Looks good to me, approved
jenkins-bot: Verified
diff --git a/lib/api/ParsoidService.js b/lib/api/ParsoidService.js
index a1c962b..3f471bf 100644
--- a/lib/api/ParsoidService.js
+++ b/lib/api/ParsoidService.js
@@ -6,6 +6,7 @@
// global includes
var express = require('express');
+var finalHandler = require('finalhandler');
var compression = require('compression');
var hbs = require('express-handlebars');
var favicon = require('serve-favicon');
@@ -111,6 +112,22 @@
next();
});
+ // Log unhandleds errors passed along with our logger.
+ var logError = function(err, req, res) {
+ var logger = res.locals.env ? res.locals.env.logger :
processLogger;
+ var args = ['warning', req.method, req.originalUrl, err];
+ if (err.type === 'entity.too.large') {
+ // Add the expected length of the stream.
+ args.push('expected: ' + err.expected);
+ }
+ logger.log.apply(logger, args);
+ };
+
+ app.use(function(err, req, res, next) {
+ var done = finalHandler(req, res, { onerror: logError });
+ done(err);
+ });
+
// Catch errors
app.on('error', function(err) {
if (err.errno === 'EADDRINUSE') {
diff --git a/lib/api/apiUtils.js b/lib/api/apiUtils.js
index e62b0f7..345ad74 100644
--- a/lib/api/apiUtils.js
+++ b/lib/api/apiUtils.js
@@ -20,9 +20,11 @@
*/
var apiUtils = module.exports = {
/** @property {string} */
- WIKITEXT_CONTENT_TYPE:
'text/plain;profile="mediawiki.org/specs/wikitext/1.0.0";charset=utf-8',
+ WIKITEXT_CONTENT_TYPE: 'text/plain; charset=utf-8;
profile="mediawiki.org/specs/wikitext/1.0.0"',
/** @property {string} */
- HTML_CONTENT_TYPE:
'text/html;profile="mediawiki.org/specs/html/1.1.0";charset=utf-8',
+ HTML_CONTENT_TYPE: 'text/html; charset=utf-8;
profile="mediawiki.org/specs/html/1.1.0"',
+ /** @property {string} */
+ DATA_PARSOID_CONTENT_TYPE: 'application/json; charset=utf-8;
profile="mediawiki.org/specs/data-parsoid/0.0.1"',
};
/**
@@ -63,24 +65,6 @@
return;
} else {
res.setHeader(name, value);
- }
-};
-
-/**
- * End response, but only if response hasn't been sent.
- *
- * @method
- * @param {Response} res The response object from our routing function.
- * @param {MWParserEnvironment} env
- * @param {String|Buffer} [out]
- */
-apiUtils.endResponse = function(res, env, out) {
- if (env.responseSent) {
- return;
- } else {
- env.responseSent = true;
- res.end(out);
- env.log("end/response");
}
};
@@ -495,7 +479,7 @@
innerXML: res.locals.bodyOnly && res.locals.apiVersion
> 2,
}).str;
apiUtils.setHeader(res, env, 'content-type',
apiUtils.HTML_CONTENT_TYPE);
- apiUtils.endResponse(res, env, output);
+ apiUtils.sendResponse(res, env, output);
}
if (timer) {
diff --git a/lib/api/routes.js b/lib/api/routes.js
index 3ddaf2c..f5f8848 100644
--- a/lib/api/routes.js
+++ b/lib/api/routes.js
@@ -126,7 +126,8 @@
apiUtils.sendResponse(res, env,
logData.fullMsg(), logData.flatLogObject().code || 500);
}).catch(function(e) {
console.error(e.stack || e);
- res.end(e.stack || e);
+ res.end();
+ return Promise.reject(e);
}).nodify(callback);
}
return Promise.resolve().nodify(callback);
@@ -183,7 +184,7 @@
// robots.txt: no indexing.
routes.robots = function(req, res) {
- res.end("User-agent: *\nDisallow: /\n");
+ res.send("User-agent: *\nDisallow: /\n");
};
// Return Parsoid version based on package.json + git sha1 if available
@@ -444,7 +445,7 @@
.then(apiUtils.endHtml2wt)
.then(function(output) {
apiUtils.setHeader(res, env, 'content-type',
apiUtils.WIKITEXT_CONTENT_TYPE);
- apiUtils.endResponse(res, env, output);
+ apiUtils.sendResponse(res, env, output);
});
return apiUtils.cpuTimeout(p, res)
.catch(apiUtils.timeoutResp.bind(null, env));
diff --git a/lib/wt2html/DOMPostProcessor.js b/lib/wt2html/DOMPostProcessor.js
index b49d0d6..a0b64a2 100644
--- a/lib/wt2html/DOMPostProcessor.js
+++ b/lib/wt2html/DOMPostProcessor.js
@@ -9,6 +9,7 @@
var util = require('util');
var DOMTraverser = require('../utils/DOMTraverser.js').DOMTraverser;
var DU = require('../utils/DOMUtils.js').DOMUtils;
+var apiUtils = require('../api/apiUtils.js');
var dumpDOM = require('./pp/dumper.js').dumpDOM;
var CleanUp = require('./pp/cleanup.js');
var computeDSR = require('./pp/computeDSR.js').computeDSR;
@@ -346,7 +347,7 @@
var script = document.createElement("script");
DU.addAttributes(script, {
id: "mw-data-parsoid",
- type:
'application/json;profile="mediawiki.org/specs/data-parsoid/0.0.1"',
+ type: apiUtils.DATA_PARSOID_CONTENT_TYPE,
});
script.appendChild(document.createTextNode(dp));
document.head.appendChild(script);
diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json
index 444ade7..5802302 100644
--- a/npm-shrinkwrap.json
+++ b/npm-shrinkwrap.json
@@ -586,18 +586,6 @@
"from": "https://registry.npmjs.org/etag/-/etag-1.7.0.tgz";,
"resolved": "https://registry.npmjs.org/etag/-/etag-1.7.0.tgz";
},
- "finalhandler": {
- "version": "0.4.0",
- "from":
"https://registry.npmjs.org/finalhandler/-/finalhandler-0.4.0.tgz";,
- "resolved":
"https://registry.npmjs.org/finalhandler/-/finalhandler-0.4.0.tgz";,
- "dependencies": {
- "unpipe": {
- "version": "1.0.0",
- "from": "https://registry.npmjs.org/unpipe/-/unpipe-1.0.0.tgz";,
- "resolved":
"https://registry.npmjs.org/unpipe/-/unpipe-1.0.0.tgz";
- }
- }
- },
"fresh": {
"version": "0.3.0",
"from": "https://registry.npmjs.org/fresh/-/fresh-0.3.0.tgz";,
@@ -903,6 +891,47 @@
}
}
},
+ "finalhandler": {
+ "version": "0.4.0",
+ "from":
"https://registry.npmjs.org/finalhandler/-/finalhandler-0.4.0.tgz";,
+ "resolved":
"https://registry.npmjs.org/finalhandler/-/finalhandler-0.4.0.tgz";,
+ "dependencies": {
+ "debug": {
+ "version": "2.2.0",
+ "from": "https://registry.npmjs.org/debug/-/debug-2.2.0.tgz";,
+ "resolved": "https://registry.npmjs.org/debug/-/debug-2.2.0.tgz";,
+ "dependencies": {
+ "ms": {
+ "version": "0.7.1",
+ "from": "https://registry.npmjs.org/ms/-/ms-0.7.1.tgz";,
+ "resolved": "https://registry.npmjs.org/ms/-/ms-0.7.1.tgz";
+ }
+ }
+ },
+ "escape-html": {
+ "version": "1.0.2",
+ "from":
"https://registry.npmjs.org/escape-html/-/escape-html-1.0.2.tgz";,
+ "resolved":
"https://registry.npmjs.org/escape-html/-/escape-html-1.0.2.tgz";
+ },
+ "on-finished": {
+ "version": "2.3.0",
+ "from":
"https://registry.npmjs.org/on-finished/-/on-finished-2.3.0.tgz";,
+ "resolved":
"https://registry.npmjs.org/on-finished/-/on-finished-2.3.0.tgz";,
+ "dependencies": {
+ "ee-first": {
+ "version": "1.1.1",
+ "from":
"https://registry.npmjs.org/ee-first/-/ee-first-1.1.1.tgz";,
+ "resolved":
"https://registry.npmjs.org/ee-first/-/ee-first-1.1.1.tgz";
+ }
+ }
+ },
+ "unpipe": {
+ "version": "1.0.0",
+ "from": "https://registry.npmjs.org/unpipe/-/unpipe-1.0.0.tgz";,
+ "resolved": "https://registry.npmjs.org/unpipe/-/unpipe-1.0.0.tgz";
+ }
+ }
+ },
"gelf-stream": {
"version": "0.2.4",
"from": "https://registry.npmjs.org/gelf-stream/-/gelf-stream-0.2.4.tgz";,
diff --git a/package.json b/package.json
index b6bf015..0a6b052 100644
--- a/package.json
+++ b/package.json
@@ -15,6 +15,7 @@
"entities": "~1.1.1",
"express": "~4.13.3",
"express-handlebars": "~2.0.1",
+ "finalhandler": "~0.4.0",
"gelf-stream": "~0.2.4",
"html5": "~1.0.5",
"node-txstatsd": "~0.1.5",
diff --git a/tests/mocha/api.js b/tests/mocha/api.js
index bd450cf..675c8a4 100644
--- a/tests/mocha/api.js
+++ b/tests/mocha/api.js
@@ -155,7 +155,7 @@
res.statusCode.should.equal(200);
res.headers.should.have.property('content-type');
res.headers['content-type'].should.equal(
-
'text/html;profile="mediawiki.org/specs/html/1.1.0";charset=utf-8'
+ 'text/html; charset=utf-8;
profile="mediawiki.org/specs/html/1.1.0"'
);
var doc =
domino.createDocument(res.text);
if (expectFunc) {
@@ -172,14 +172,14 @@
res.body.html.should.have.property('headers');
res.body.html.headers.should.have.property('content-type');
res.body.html.headers['content-type'].should.equal(
-
'text/html;profile="mediawiki.org/specs/html/1.1.0";charset=utf-8'
+ 'text/html; charset=utf-8;
profile="mediawiki.org/specs/html/1.1.0"'
);
res.body.html.should.have.property('body');
res.body.should.have.property('data-parsoid');
res.body['data-parsoid'].should.have.property('headers');
res.body['data-parsoid'].headers.should.have.property('content-type');
res.body['data-parsoid'].headers['content-type'].should.equal(
-
'application/json;profile="mediawiki.org/specs/data-parsoid/0.0.1"'
+ 'application/json;
charset=utf-8; profile="mediawiki.org/specs/data-parsoid/0.0.1"'
);
res.body['data-parsoid'].should.have.property('body');
var doc =
domino.createDocument(res.body.html.body);
--
To view, visit https://gerrit.wikimedia.org/r/252607
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibc870e15c58b45f2cf9428d8d5541480c8b8e469
Gerrit-PatchSet: 5
Gerrit-Project: mediawiki/services/parsoid
Gerrit-Branch: master
Gerrit-Owner: Arlolra <[email protected]>
Gerrit-Reviewer: Arlolra <[email protected]>
Gerrit-Reviewer: Cscott <[email protected]>
Gerrit-Reviewer: Subramanya Sastry <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
