Skip to content

Commit

Permalink
client: handle errors in connectAndInspect
Browse files Browse the repository at this point in the history
As `DataCollector` always returns the data it managed to collect
successfully and passes errors as a separate object, `connectAndInspect`
has put errors as `_errors` property of the result to be consistent.
This was a horrible design decision that was counter-intuitive and would
only lead to the lack of proper error handling in application code (and
has actually led in our integration test).

Worse than that, even this `_errors` property would never have been
populated with errors because of a bug in `connectAndInspect`.  A patch
for the bug (just in case we ever want to backport it):

diff --git a/lib/client.js b/lib/client.js
index 83236d9..b0436fd 100644
--- a/lib/client.js
+++ b/lib/client.js
@@ -124,7 +124,7 @@ Client.prototype.connectAndInspect = function(
       interfaces.forEach((interfaceName) => {
         connection.inspectInterface(interfaceName, (error, appInterface) => {
           if (error) {
-            appInterface = null;
+            appInterface = error;
           }
           collector.collect(interfaceName, appInterface);
         });

The proper solution would have been to stop inspecting interfaces as
soon as the first error occurs and return it as the first argument of
the callback.  As it turned out, it was quite tricky to do with
DataCollector, and some other issues were found while inspecting
`metasync` sources too.  Fixing these problems requires
backwards-incompatible changes and will certainly happen in the next
semver-major release of `metasync`, but we need a quick fix here.  For
this reason, the function was refactored to use promises.

PR-URL: #105
Reviewed-by: Mykola Bilochub <nbelochub@gmail.com>
  • Loading branch information
aqrln committed Mar 21, 2017
1 parent b6e9eee commit d8ba3fe
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 18 deletions.
38 changes: 21 additions & 17 deletions lib/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
const events = require('events');
const util = require('util');

const metasync = require('metasync');

const apps = require('./applications');
const common = require('./common');
const Connection = require('./connection');
Expand Down Expand Up @@ -112,22 +110,28 @@ Client.prototype.connectAndInspect = function(
return callback(error);
}

const collector = new metasync.DataCollector(interfaces.length);

collector.on('done', (errs, api) => {
if (errs) {
api._errors = errs;
}
// DataCollector from metarhia/MetaSync is a better abstraction to do
// this kind of things, but there are some issues that must be resolved
// prior to using it here.
Promise.all(
interfaces.map((name) => new Promise((resolve, reject) => {
connection.inspectInterface(name, (error, proxy) => {
if (error) {
reject(error);
} else {
resolve(proxy);
}
});
}))
).then((proxies) => {
const api = proxies.reduce((acc, proxy, idx) => {
const name = interfaces[idx];
acc[name] = proxy;
return acc;
}, Object.create(null));
callback(null, connection, api);
});

interfaces.forEach((interfaceName) => {
connection.inspectInterface(interfaceName, (error, appInterface) => {
if (error) {
appInterface = null;
}
collector.collect(interfaceName, appInterface);
});
}).catch((error) => {
callback(error);
});
}
);
Expand Down
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
"./lib/simple-auth.js": false
},
"dependencies": {
"metasync": "0.1.x",
"websocket": "^1.0.24"
},
"devDependencies": {
Expand Down

0 comments on commit d8ba3fe

Please sign in to comment.