Change Query.idMap to count ids, to be defensive against duplicate ids in query results #253
Conversation
…s in query results (see share/sharedb-mongo#55)
| @@ -158,7 +158,7 @@ function Query(model, collectionName, expression, options) { | |||
| // because otherwise maybeUnload would be looping through the entire results | |||
| // set of each query on the same collection for every doc checked | |||
| // | |||
| // Map of id -> true | |||
| // Map of id -> count of ids | |||
| this.idMap = {}; | |||
There was a problem hiding this comment.
I also noticed that subscriptions.js does a plain property check on idMap instead of using hasOwnProperty, which means that the check will be incorrect if an id happens to be the name of an Object prototype property. Do you think that matters enough to fix? Racer-generated ids are UUIDs, so it's only a problem for manually-specified ids.
There was a problem hiding this comment.
True. The problem would result in a doc not getting locally cleaned up, so honestly, the impact isn't likely to be a concern. I'd probably not worry about it for now.
There was a problem hiding this comment.
Actually, since idMap is now a number, we could use the check idMap[id] > 0, which might be better since it is more explicit.
lib/Model/Query.js
Outdated
| @@ -312,7 +317,8 @@ Query.prototype._removeMapIds = function(ids) { | |||
| Query.prototype._addMapIds = function(ids) { | |||
| for (var i = ids.length; i--;) { | |||
| var id = ids[i]; | |||
| this.idMap[id] = true; | |||
| this.idMap[id] = this.idMap[id] || 0; | |||
| this.idMap[id]++; | |||
There was a problem hiding this comment.
Equivalently, you can write this: this.idMap[id] = (this.idMap[id] || 0) + 1;
It is slightly more complexity on one line, and I don't even know if it makes a difference once optimized, but avoiding two different property gets + assignments might be better.
lib/Model/Query.js
Outdated
| } | ||
| if (this.idMap[id] === 0) { | ||
| delete this.idMap[id]; | ||
| } |
There was a problem hiding this comment.
I think it is slightly more clear to write:
if (this.idMap[id] > 1) {
this.idMap[id]--;
} else {
delete this.idMap[id];
}
There was a problem hiding this comment.
The two are not quite the same.
In your proposal, any ids that hit zero wouldn't get cleaned up in the map unless _removeMapIds happened to get called again with one of those zero-ed out ids.
There was a problem hiding this comment.
Oh wait, never mind. The condition is > 1.
There was a problem hiding this comment.
Ok, this is done now too, thanks for the suggestion! It's one fewer conditional check too, which is nice.
| @@ -158,7 +158,7 @@ function Query(model, collectionName, expression, options) { | |||
| // because otherwise maybeUnload would be looping through the entire results | |||
| // set of each query on the same collection for every doc checked | |||
| // | |||
| // Map of id -> true | |||
| // Map of id -> count of ids | |||
| this.idMap = {}; | |||
There was a problem hiding this comment.
Actually, since idMap is now a number, we could use the check idMap[id] > 0, which might be better since it is more explicit.
|
LGTM! |
There's a ShareDB issue where a subscribed ShareDB query can emit an insert event with an id that's already present in the result list. ShareDB then immediately emits an event (usually remove) to fix the duplication, but because the Racer query's
idMapdidn't keep track of id counts, it would mistakenly remove the id when the id's count went from 2 to 1. That causes the doc to become unloaded, since Racer uses theidMapto determine if any queries are subscribed to the doc.The underlying ShareDB issue requires more investigation to fix, but Racer should still be defensive about it, especially since the issue results in Racer becoming internally inconsistent: