From 908a5a2408b96e60986ecc48c42c0d6037cab489 Mon Sep 17 00:00:00 2001 From: deilann Date: Wed, 5 Feb 2014 13:07:56 -0800 Subject: [PATCH 1/5] updating tavern link to new issue ticket Explains how Github works. Should reduce number of dupe tickets. --- views/options/social/tavern.jade | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/views/options/social/tavern.jade b/views/options/social/tavern.jade index 349065f739..c619c1205d 100644 --- a/views/options/social/tavern.jade +++ b/views/options/social/tavern.jade @@ -122,6 +122,6 @@ include ../../shared/formatting-help small(style='position: relative; top: 18px; left: 0px') alert.alert-info - !=env.t('tavernAlert1') + ' ' + env.t('tavernAlert2') + '.' + !=env.t('tavernAlert1') + ' ' + env.t('tavernAlert2') + '.' ul.unstyled.tavern-chat include ./chat-message From bf5e9016a4cb7889b3a9e39b90eb35cb8f7f9ec8 Mon Sep 17 00:00:00 2001 From: Tyler Renelle Date: Wed, 5 Feb 2014 17:26:09 -0700 Subject: [PATCH 2/5] fix(errors): `return next(err)` when experiencing errors, instead of res.json(500,{err:err}). Let the top-level error handler handle this (needed for upcoming versionerror discarding) --- src/controllers/user.js | 14 +++++++------- src/utils.js | 30 +++++++++++++++--------------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/controllers/user.js b/src/controllers/user.js index a44f5937da..45b093e85e 100644 --- a/src/controllers/user.js +++ b/src/controllers/user.js @@ -81,7 +81,7 @@ api.score = function(req, res, next) { var delta = user.ops.score({params:{id:task.id, direction:direction}}); user.save(function(err,saved){ - if (err) return res.json(500, {err: err}); + if (err) return next(err); // TODO this should be return {_v,task,stats,_tmp}, instead of merging everything togther at top-level response // However, this is the most commonly used API route, and changing it will mess with all 3rd party consumers. Bad idea :( res.json(200, _.extend({ @@ -193,7 +193,7 @@ api.update = function(req, res, next) { }); user.save(function(err) { if (!_.isEmpty(errors)) return res.json(401, {err: errors}); - if (err) return res.json(500, {err: err}); + if (err) return next(err); res.json(200, user); }); }; @@ -235,7 +235,7 @@ api.cron = function(req, res, next) { api['delete'] = function(req, res) { res.locals.user.remove(function(err){ - if (err) return res.json(500,{err:err}); + if (err) return next(err); res.send(200); }) } @@ -258,7 +258,7 @@ api.addTenGems = function(req, res) { var user = res.locals.user; user.balance += 2.5; user.save(function(err){ - if (err) return res.json(500,{err:err}); + if (err) return next(err); res.send(204); }) } @@ -381,7 +381,7 @@ api.cast = function(req, res) { var done = function(){ var err = arguments[0]; var saved = _.size(arguments == 3) ? arguments[2] : arguments[1]; - if (err) return res.json(500, {err:err}); + if (err) return next(err); res.json(saved); } @@ -445,12 +445,12 @@ _.each(shared.wrap({}).ops, function(op,k){ res.locals.user.ops[k](req,function(err, response){ // If we want to send something other than 500, pass err as {code: 200, message: "Not enough GP"} if (err) { - if (!err.code) return res.json(500,{err:err}); + if (!err.code) return next(err); if (err.code >= 400) return res.json(err.code,{err:err.message}); // In the case of 200s, they're friendly alert messages like "You're pet has hatched!" - still send the op } res.locals.user.save(function(err){ - if (err) return res.json(500,{err:err}); + if (err) return next(err); res.json(200,response); }) }) diff --git a/src/utils.js b/src/utils.js index 1fb45587af..a4a44d52c2 100644 --- a/src/utils.js +++ b/src/utils.js @@ -49,21 +49,21 @@ module.exports.setupConfig = function(){ }; module.exports.crashWorker = function(server,mongoose) { - return function(err, req, res, next) { - if (!cluster.isMaster) { - // make sure we close down within 30 seconds - var killtimer = setTimeout(function() { - process.exit(1); - }, 30000); - // But don't keep the process open just for that! - killtimer.unref(); - // stop taking new requests. - server.close(); - mongoose.connection.close(); - cluster.worker.disconnect(); - } - next(err); - }; + return function(err, req, res, next) { + if (!cluster.isMaster) { + // make sure we close down within 30 seconds + var killtimer = setTimeout(function() { + process.exit(1); + }, 30000); + // But don't keep the process open just for that! + killtimer.unref(); + // stop taking new requests. + server.close(); + mongoose.connection.close(); + cluster.worker.disconnect(); + } + next(err); + }; } From 4c3c4b2ae788db0a301ce84c617e201d7d1c065e Mon Sep 17 00:00:00 2001 From: Tyler Renelle Date: Wed, 5 Feb 2014 17:31:37 -0700 Subject: [PATCH 3/5] perf(user): save user.save till very end of ops queue --- src/controllers/user.js | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/src/controllers/user.js b/src/controllers/user.js index 45b093e85e..6780e50620 100644 --- a/src/controllers/user.js +++ b/src/controllers/user.js @@ -473,28 +473,33 @@ api.batchUpdate = function(req, res, next) { var oldSend = res.send; var oldJson = res.json; - var callOp = function(_req, cb) { - res.send = res.json = function(code, data) { - if (_.isNumber(code) && code >= 500) - return cb(code+": "+ (data.message ? data.message : data.err ? data.err : JSON.stringify(data))); - return cb(); - }; - api[_req.op](_req, res); - }; + // Stash user.save, we'll queue the save op till the end (so we don't overload the server) + var oldSave = user.save; + user.save = function(cb){cb(null,user)} - res.locals.ops = []; // Setup the array of functions we're going to call in parallel with async - var ops = _.transform(req.body, function(result, _req) { - if (!_.isEmpty(_req)) { - result.push(function(cb) { - res.locals.ops.push(_req); - callOp(_req, cb); - }); - } + res.locals.ops = []; + var ops = _.transform(req.body, function(m,_req){ + if (_.isEmpty(_req)) return; + m.push(function() { + var cb = arguments[arguments.length-1]; + res.locals.ops.push(_req); + res.send = res.json = function(code, data) { + if (_.isNumber(code) && code >= 500) + return cb(code+": "+ (data.message ? data.message : data.err ? data.err : JSON.stringify(data))); + return cb(); + }; + api[_req.op](_req, res); + }); + }) + // Finally, save user at the end + .concat(function(){ + user.save = oldSave; + user.save(arguments[arguments.length-1]); }); // call all the operations, then return the user object to the requester - async.series(ops, function(err) { + async.waterfall(ops, function(err,user) { res.json = oldJson; res.send = oldSend; if (err) return next(err); From 0c21f54c67b52b07c417fd8216c6b04bce59d0ab Mon Sep 17 00:00:00 2001 From: Tyler Renelle Date: Wed, 5 Feb 2014 21:28:46 -0700 Subject: [PATCH 4/5] fix(user): make sure next is passed to all routes, and is available in err-back of batch updates --- src/controllers/user.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/controllers/user.js b/src/controllers/user.js index 6780e50620..f580f0f360 100644 --- a/src/controllers/user.js +++ b/src/controllers/user.js @@ -34,7 +34,7 @@ api.getContent = function(req, res, next) { --------------- */ -findTask = function(req, res) { +findTask = function(req, res, next) { return task = res.locals.user.tasks[req.params.id]; }; @@ -233,7 +233,7 @@ api.cron = function(req, res, next) { // api.reroll // Shared.ops // api.reset // Shared.ops -api['delete'] = function(req, res) { +api['delete'] = function(req, res, next) { res.locals.user.remove(function(err){ if (err) return next(err); res.send(200); @@ -254,7 +254,7 @@ api['delete'] = function(req, res) { ------------------------------------------------------------------------ */ -api.addTenGems = function(req, res) { +api.addTenGems = function(req, res, next) { var user = res.locals.user; user.balance += 2.5; user.save(function(err){ @@ -268,7 +268,7 @@ api.addTenGems = function(req, res) { /* Setup Stripe response when posting payment */ -api.buyGems = function(req, res) { +api.buyGems = function(req, res, next) { var api_key = nconf.get('STRIPE_API_KEY'); var stripe = require("stripe")(api_key); var token = req.body.id; @@ -312,7 +312,7 @@ api.buyGems = function(req, res) { }); }; -api.cancelSubscription = function(req, res) { +api.cancelSubscription = function(req, res, next) { var api_key = nconf.get('STRIPE_API_KEY'); var stripe = require("stripe")(api_key); var user = res.locals.user; @@ -370,7 +370,7 @@ api.buyGemsPaypalIPN = function(req, res, next) { Spells ------------------------------------------------------------------------ */ -api.cast = function(req, res) { +api.cast = function(req, res, next) { var user = res.locals.user; var targetType = req.query.targetType; var targetId = req.query.targetId; @@ -489,7 +489,7 @@ api.batchUpdate = function(req, res, next) { return cb(code+": "+ (data.message ? data.message : data.err ? data.err : JSON.stringify(data))); return cb(); }; - api[_req.op](_req, res); + api[_req.op](_req, res, cb); }); }) // Finally, save user at the end From e268839bcb87580b6eec2742081f3ffed43ddd83 Mon Sep 17 00:00:00 2001 From: Tyler Renelle Date: Wed, 5 Feb 2014 21:33:24 -0700 Subject: [PATCH 5/5] chore(throttling): lower api limit a tad, just to test --- src/middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware.js b/src/middleware.js index 781b6f7202..4450133f01 100644 --- a/src/middleware.js +++ b/src/middleware.js @@ -12,7 +12,7 @@ module.exports.apiThrottle = function(app) { catagories:{ normal: { // 2 req/s, but split as minutes - totalRequests: 120, + totalRequests: 80, every: 60000 } }