From ec5b0fea393cbe44da4d965c31edbbd7372c9042 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 14 Jul 2017 05:22:34 +0300 Subject: [PATCH] MXS-1300: Use native promises for requests The request-promise-native package adds support for request with native promises. This is a convenient way to synchronize multiple HTTP requests. The functions are changed to use promises to make testing easier. With promises, the testing code can use the chai-as-promised library. There are a few cases where doRequest is called with a callback that again calls doAsyncRequest. With multiple hosts, this causes each command to be propagated to all servers. This is a design flaw of the current multi-host mode and needs to be changed. Changed maxctrl.js to wait on the promises to silence some warnings if the promise rejections are ignored. --- maxctrl/common.js | 112 ++++++++++++++++++++++------------------- maxctrl/core.js | 8 +-- maxctrl/lib/call.js | 10 ++-- maxctrl/lib/disable.js | 2 +- maxctrl/lib/enable.js | 2 +- maxctrl/lib/link.js | 4 +- maxctrl/lib/unlink.js | 4 +- maxctrl/maxctrl.js | 16 +++--- maxctrl/package.json | 1 + 9 files changed, 79 insertions(+), 80 deletions(-) diff --git a/maxctrl/common.js b/maxctrl/common.js index b1114d512..7b874d9f6 100644 --- a/maxctrl/common.js +++ b/maxctrl/common.js @@ -11,7 +11,7 @@ * Public License. */ -var request = require('request'); +var request = require('request-promise-native'); var colors = require('colors/safe'); var Table = require('cli-table'); @@ -127,46 +127,63 @@ module.exports = function() { return base + argv.user + ':' + argv.password + '@' + host + '/v1/' + endpoint } - // Helper for executing requests and handling their responses - this.doRequest = function(resource, cb, obj) { - pingCluster(this.argv.hosts) - .then(function() { - var argv = this.argv - argv.hosts.forEach(function(host) { - args = obj || {} - args.uri = getUri(host, argv.secure, resource) - args.json = true - args.timeout = argv.timeout + // Helper for executing requests and handling their responses, returns a + // promise that is fulfilled when all requests successfully complete. The + // promise is rejected if any of the requests fails. + this.doAsyncRequest = function(resource, cb, obj) { + return new Promise(function (resolve, reject) { + pingCluster(this.argv.hosts) + .then(function() { - request(args, function(err, resp, res) { - if (err) { - // Failed to request - console.log(colors.yellow(host) + ':') - logError(JSON.stringify(err, null, 4)) - } else if (resp.statusCode == 200 && cb) { - // Request OK, returns data - if (!argv.tsv) { + var promises = [] + + this.argv.hosts.forEach(function(host) { + args = obj || {} + args.uri = getUri(host, this.argv.secure, resource) + args.json = true + args.timeout = this.argv.timeout + + var p = request(args) + .then(function(res) { + if (res && cb) { + // Request OK, returns data + if (!this.argv.tsv) { + console.log(colors.yellow(host) + ':') + } + return cb(res) + } else { + // Request OK, no data or data is ignored + console.log(colors.yellow(host) + ': ' + colors.green('OK')) + } + return Promise.resolve() + }, function(err) { console.log(colors.yellow(host) + ':') - } - cb(res) - } else if (resp.statusCode == 204) { - // Request OK, no data - console.log(colors.yellow(host) + ': ' + colors.green('OK')) - } else { - // Unexpected return code, probably an error - var errstr = resp.statusCode + ' ' + resp.statusMessage - if (res) { - errstr += ' ' + JSON.stringify(res, null, 4) - } - console.log(colors.yellow(host) + ':') - logError(errstr) - } + if (err.response.body) { + logError(JSON.stringify(err.response.body, null, 4)) + } else { + logError('Server responded with ' + err.statusCode) + } + return Promise.reject() + }) + + // Push the request on to the stack + promises.push(p) }) + + // Wait for all promises to resolve or reject + Promise.all(promises) + .then(resolve, reject) + }, function(err) { + // One of the HTTP request pings to the cluster failed, log the error + logError(JSON.stringify(err.error, null, 4)) + reject() }) - }) - .catch(function(err) { - logError(JSON.stringify(err, null, 4)) - }) + }) + } + + this.doRequest = function(resource, cb, obj) { + return doAsyncRequest(resource, cb, obj) + .then(this.argv.resolve, this.argv.reject) } this.updateValue = function(resource, key, value) { @@ -178,6 +195,11 @@ module.exports = function() { this.logError = function(err) { console.log(colors.red('Error:'), err) } + + this.error = function(err) { + console.log(colors.red('Error:'), err) + this.argv.reject() + } } var tsvopts = { @@ -230,25 +252,11 @@ function getTable(headobj) { return new Table(opts) } -function pingMaxScale(host) { - return new Promise(function(resolve, reject) { - request('http://' + host + '/v1', function(err, resp, res) { - if (err) { - reject(err) - } else if (resp.statusCode != 200) { - reject(resp.statusCode + ' ' + resp.statusMessage) - } else { - resolve() - } - }) - }) -} - function pingCluster(hosts) { var promises = [] hosts.forEach(function(i) { - promises.push(pingMaxScale(i)) + promises.push(request('http://' + i + '/v1')) }) return Promise.all(promises) diff --git a/maxctrl/core.js b/maxctrl/core.js index 21dacbe64..2fb142d34 100644 --- a/maxctrl/core.js +++ b/maxctrl/core.js @@ -82,12 +82,6 @@ module.exports = function(argv) { .command('*', 'the default command', {}, () => { console.log('Unknown command. See output of `help` for a list of commands.') }) - .parse(process.argv, function(err, argv, output) { - if (err) { - reject(output) - } else { - resolve(output); - } - }) + .parse(argv, {resolve: resolve, reject: reject}) }) } diff --git a/maxctrl/lib/call.js b/maxctrl/lib/call.js index 80fd6538c..c42833d65 100644 --- a/maxctrl/lib/call.js +++ b/maxctrl/lib/call.js @@ -32,11 +32,11 @@ exports.builder = function(yargs) { } }) - maxctrl - .doRequest('maxscale/modules/' + argv.module + '/' + argv.command + '?' + argv.parameters.join('&'), - function(resp) { - console.log(JSON.stringify(resp, null, 4)) - }, { method: verb }) + return maxctrl + .doAsyncRequest('maxscale/modules/' + argv.module + '/' + argv.command + '?' + argv.parameters.join('&'), + function(resp) { + console.log(JSON.stringify(resp, null, 4)) + }, { method: verb }) }) }) .usage('Usage: call ') diff --git a/maxctrl/lib/disable.js b/maxctrl/lib/disable.js index 5c53014e9..26744441f 100644 --- a/maxctrl/lib/disable.js +++ b/maxctrl/lib/disable.js @@ -30,7 +30,7 @@ exports.builder = function(yargs) { .updateValue('maxscale/logs', 'data.attributes.parameters.log_' + argv.log, false) } else { maxctrl(argv) - .logError('Invalid log priority: ' + argv.log); + .error('Invalid log priority: ' + argv.log); } }) .command('maxlog', 'Disable MaxScale logging', {}, function(argv) { diff --git a/maxctrl/lib/enable.js b/maxctrl/lib/enable.js index 85a757e38..c87943315 100644 --- a/maxctrl/lib/enable.js +++ b/maxctrl/lib/enable.js @@ -30,7 +30,7 @@ exports.builder = function(yargs) { .updateValue('maxscale/logs', 'data.attributes.parameters.log_' + argv.log, true) } else { maxctrl(argv) - .logError('Invalid log priority: ' + argv.log); + .error('Invalid log priority: ' + argv.log); } }) .command('maxlog', 'Enable MaxScale logging', {}, function(argv) { diff --git a/maxctrl/lib/link.js b/maxctrl/lib/link.js index 2955f4677..d7011c761 100644 --- a/maxctrl/lib/link.js +++ b/maxctrl/lib/link.js @@ -25,8 +25,8 @@ function addServer(argv, path, targets) { _.set(res, 'data.relationships.servers.data', servers) delete res.data.attributes - maxctrl(argv) - .doRequest(path, null, {method: 'PATCH', body: res}) + return maxctrl(argv) + .doAsyncRequest(path, null, {method: 'PATCH', body: res}) }) } diff --git a/maxctrl/lib/unlink.js b/maxctrl/lib/unlink.js index e8f38e5f4..c1aab0da2 100644 --- a/maxctrl/lib/unlink.js +++ b/maxctrl/lib/unlink.js @@ -25,8 +25,8 @@ function removeServer(argv, path, targets) { _.set(res, 'data.relationships.servers.data', servers) delete res.data.attributes - maxctrl(argv) - .doRequest(path, null, {method: 'PATCH', body: res}) + return maxctrl(argv) + .doAsyncRequest(path, null, {method: 'PATCH', body: res}) }) } diff --git a/maxctrl/maxctrl.js b/maxctrl/maxctrl.js index 0e576355e..724ef6289 100644 --- a/maxctrl/maxctrl.js +++ b/maxctrl/maxctrl.js @@ -13,18 +13,14 @@ 'use strict'; -var argv = process.argv +var maxctrl = require('./core.js') // Mangle the arguments if we are being called from the command line -if (argv[0] == process.execPath) { - argv.shift() +if (process.argv[0] == process.execPath) { + process.argv.shift() // The first argument is always the script - argv.shift() + process.argv.shift() } -require('./core.js')(argv) - .then(function(output){ - if (output.length > 0) { - console.log(output) - } - }, console.log) +maxctrl(process.argv) + .then(function(out) {}, function(out) {}) diff --git a/maxctrl/package.json b/maxctrl/package.json index 791ae18c7..247c7d073 100644 --- a/maxctrl/package.json +++ b/maxctrl/package.json @@ -20,6 +20,7 @@ "lodash": "^4.17.4", "lodash-getpath": "^0.2.4", "request": "^2.81.0", + "request-promise-native": "^1.0.3", "yargs": "^8.0.2" } }