namespace_browser: Possibly fixing the flakyness.
I believe this is the cause:
Tests assume, rightly so, no state mutation happens after
the end event. hasChildrenPromise listens to data event
and mutates a state (item.isGlobbable.set(true);) and cancels
the context, but if a new data comes in before RPC is cancelled
chances are end event has already fired and now we are mutating
state again. Simple switch to .once() shoud fix this.
Also:
-Realizing Promise.all does not do what I expected in
some cases and replacing it with Bluebird.settle (although
based on the current state of code .all and .settle should
behave the same, technically .settle is the right API to use)
-Renaming streamError to globError
-Making tests print more when they fail
Change-Id: I3e7afd7a001b22394a8dc1eff997f1ec21237f44
diff --git a/package.json b/package.json
index 2aba86a..1709f2b 100644
--- a/package.json
+++ b/package.json
@@ -32,6 +32,7 @@
"routes": "^1.2.0",
"lodash": "~3.0.0",
"vis": "~3.9.1",
- "extend": "~2.0.0"
+ "extend": "~2.0.0",
+ "bluebird": "~2.3.2"
}
}
diff --git a/src/components/browse/items/index.js b/src/components/browse/items/index.js
index 900dfc3..8af370f 100644
--- a/src/components/browse/items/index.js
+++ b/src/components/browse/items/index.js
@@ -113,7 +113,7 @@
}
state.put('items', items);
items.events.once('end', loadingFinished);
- items.events.on('streamError', loadingFinished);
+ items.events.on('globError', loadingFinished);
}).catch(function(err) {
log.error(err);
reject();
diff --git a/src/services/namespace/service.js b/src/services/namespace/service.js
index c8fcc34..6aea68e 100644
--- a/src/services/namespace/service.js
+++ b/src/services/namespace/service.js
@@ -1,5 +1,6 @@
var veyron = require('veyron');
var mercury = require('mercury');
+var bluebirdPromise = require('bluebird');
var vdl = require('veyron').vdl;
var LRU = require('lru-cache');
var EventEmitter = require('events').EventEmitter;
@@ -64,7 +65,7 @@
* the changes.
*
* The observable result has an events property which is an EventEmitter
- * and emits 'end' and 'streamError' events.
+ * and emits 'end' and 'globError' events.
*
* @param {string} pattern Glob pattern
* @return {Promise.<mercury.array>} Promise of an observable array
@@ -136,12 +137,15 @@
};
// Wait until all createItem promises return before triggering has ended
- Promise.all(itemPromises).then(triggerEnd).catch(triggerEnd);
+ // We are using Bluebird's settle() since Promise.all won't work as
+ // it will get rejected as soon as one is rejected but we want to know
+ // when all have resolved/rejected which is what settle() does.
+ bluebirdPromise.settle(itemPromises).then(triggerEnd).catch(triggerEnd);
});
globStream.on('error', function invalidateCacheAndLog(err) {
globCache.del(cacheKey);
- immutableResult.events.emit('streamError', err);
+ immutableResult.events.emit('globError', err);
log.error('Glob stream error for', name, err);
});
@@ -169,7 +173,7 @@
return glob(objectName).then(function(resultsObs) {
// Wait until the glob finishes before returning the item
return new Promise(function(resolve, reject) {
- resultsObs.events.on('streamError', function(err) {
+ resultsObs.events.on('globError', function(err) {
reject(err);
});
resultsObs.events.on('end', function() {
@@ -354,19 +358,19 @@
var ctx = rt.getContext().withTimeout(RPC_TIMEOUT).withCancel();
var ns = rt.namespace();
var globStream = ns.glob(ctx, namespaceUtil.join(name, '*')).stream;
- globStream.on('data', function createItem() {
+ globStream.once('data', function createItem() {
// we have at least one child
item.isGlobbable.set(true);
ctx.cancel();
resolve();
});
- globStream.on('error', function createItem(globResult) {
+ globStream.once('error', function createItem(globResult) {
//TODO(aghassemi) Certain types of error can tell us if the name
// was not accessible which can be used for the IsAccessible flag.
ctx.cancel();
resolve();
});
- globStream.on('end', function() {
+ globStream.once('end', function() {
resolve();
});
});
@@ -385,11 +389,19 @@
});
});
- var onFinishPromise = Promise.all([hasChildrenPromise, resolveNamePromise])
- .catch(function(err) {
- log.error('hasChildren or isServer failed in createNamespaceItem for ' +
- name, err);
- // we do not want to rethrow the error here. onFinish is always resolved.
+ // We are using .settle instead of .all because we don't want to resolve the
+ // onFinishPromise if one is rejected early.
+ var onFinishPromise = bluebirdPromise.settle(
+ [hasChildrenPromise, resolveNamePromise]
+ ).then(function(results) {
+ if (results[0].isRejected()) {
+ log.error('hasChildrenPromise failed in createNamespaceItem for ' +
+ name, results[0].reason());
+ }
+ if (results[1].isRejected()) {
+ log.error('resolveNamePromise failed in createNamespaceItem for ' +
+ name, results[1].reason());
+ }
});
return {
diff --git a/test/integration/services/namespace/service.js b/test/integration/services/namespace/service.js
index ec2340e..77442cd 100644
--- a/test/integration/services/namespace/service.js
+++ b/test/integration/services/namespace/service.js
@@ -37,8 +37,10 @@
t.end();
});
});
- result.events.on('streamError', t.end);
- result.events.on('itemError', t.end);
+ result.events.on('globError', function(error) {
+ t.notOk(error, 'did not expect any globs errors');
+ t.end();
+ });
}).catch(t.end);
function assertCottage(item) {
@@ -72,8 +74,10 @@
t.end();
});
});
- result.events.on('streamError', t.end);
- result.events.on('itemError', t.end);
+ result.events.on('globError', function(error) {
+ t.notOk(error, 'did not expect any globs errors');
+ t.end();
+ });
}).catch(t.end);
function assertSprinkler(item) {
@@ -105,8 +109,10 @@
t.end();
});
});
- result.events.on('streamError', t.end);
- result.events.on('itemError', t.end);
+ result.events.on('globError', function(error) {
+ t.notOk(error, 'did not expect any globs errors');
+ t.end();
+ });
}).catch(t.end);
function assertLightSwitch(item) {
@@ -145,8 +151,10 @@
t.end();
});
});
- result.events.on('streamError', t.end);
- result.events.on('itemError', t.end);
+ result.events.on('globError', function(error) {
+ t.notOk(error, 'did not expect any globs errors');
+ t.end();
+ });
}).catch(t.end);
function assertLightSwitch(item) {
@@ -180,11 +188,10 @@
t.end();
});
});
- result.events.on('streamError', function(error) {
- // we do actually expect an error in this case
+ result.events.on('globError', function(error) {
+ // we do actually expect a glob error in this case
t.ok(error);
});
- result.events.on('itemError', t.end);
}).catch(t.end);
});
