Merge pull request #276 from derbyjs/fix-unfetch-with-pending-ops

Fix memory leak when unfetching doc with pending ops
derbyjs · Mar 20, 2020 · 18bfe36 · 18bfe36
2 parents f285fe3 + 56dd54e
commit 18bfe36
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 18 deletions.
diff --git a/lib/Model/subscriptions.js b/lib/Model/subscriptions.js
@@ -168,6 +168,7 @@ Model.prototype.unsubscribeDoc = function(collectionName, id, cb) {
 // Removes the document from the local model if the model no longer has any
 // remaining fetches or subscribes via a query or direct loading
 Model.prototype._maybeUnloadDoc = function(collectionName, id) {
+  var model = this;
   var doc = this.getDoc(collectionName, id);
   if (!doc) return;
 
@@ -179,16 +180,22 @@ Model.prototype._maybeUnloadDoc = function(collectionName, id) {
   // Thus, if we remove the doc from Racer's model but don't remove it from
   // ShareDB, we can end up with an inconsistent state, with the data existing
   // in ShareDB not reflected in the racer model data
-  if (doc.shareDoc && doc.shareDoc.hasPending()) return;
+  if (doc.shareDoc && doc.shareDoc.hasPending()) {
+    doc.shareDoc.whenNothingPending(unloadDoc);
+  } else {
+    unloadDoc();
+  }
 
-  var previous = doc.get();
+  function unloadDoc() {
+    var previous = doc.get();
 
-  // Remove doc from Racer
-  this.root.collections[collectionName].remove(id);
-  // Remove doc from Share
-  if (doc.shareDoc) doc.shareDoc.destroy();
+    // Remove doc from Racer
+    model.root.collections[collectionName].remove(id);
+    // Remove doc from Share
+    if (doc.shareDoc) doc.shareDoc.destroy();
 
-  this.emit('unload', [collectionName, id], [previous, this._pass]);
+    model.emit('unload', [collectionName, id], [previous, this._pass]);
+  }
 };
 
 Model.prototype._hasDocReferences = function(collectionName, id) {

diff --git a/test/Model/loading.js b/test/Model/loading.js
@@ -2,19 +2,19 @@ var expect = require('../util').expect;
 var racer = require('../../lib/index');
 
 describe('loading', function() {
-  describe('subscribe', function() {
-    beforeEach(function(done) {
-      this.backend = racer.createBackend();
-      // Add a delay on all messages to help catch race issues
-      var delay = 5;
-      this.backend.use('receive', function(request, next) {
-        delay++;
-        setTimeout(next, delay);
-      });
-      this.model = this.backend.createModel();
-      this.model.connection.on('connected', done);
+  beforeEach(function(done) {
+    this.backend = racer.createBackend();
+    // Add a delay on all messages to help catch race issues
+    var delay = 5;
+    this.backend.use('receive', function(request, next) {
+      delay++;
+      setTimeout(next, delay);
     });
+    this.model = this.backend.createModel();
+    this.model.connection.on('connected', done);
+  });
 
+  describe('subscribe', function() {
     it('calls back simultaneous subscribes to the same document', function(done) {
       var doc = this.model.connection.get('colors', 'green');
       expect(doc.version).equal(null);
@@ -45,4 +45,31 @@ describe('loading', function() {
       });
     });
   });
+
+  describe('unfetch', function() {
+    it('unloads doc after Share doc has nothing pending', function(done) {
+      var setupModel = this.backend.createModel();
+      var model = this.model;
+      setupModel.add('colors', {id: 'green', hex: '00ff00'}, function(err) {
+        if (err) return done(err);
+
+        model.fetch('colors.green', function(err) {
+          if (err) return done(err);
+          expect(model.get('colors.green.hex')).to.equal('00ff00');
+          // Queue up a pending op.
+          model.set('colors.green.hex', '00ee00');
+          // Unfetch. The pending op prevents the doc from immedialy being unloaded.
+          model.unfetch('colors.green');
+          // Once there's nothing pending on the model/doc...
+          model.whenNothingPending(function() {
+            // Racer doc should be unloaded.
+            expect(model.get('colors.green')).to.equal(undefined);
+            // Share doc should be unloaded too.
+            expect(model.connection.getExisting('colors', 'green')).to.equal(undefined);
+            done();
+          });
+        });
+      });
+    });
+  });
 });