inProgress-team / react-native-meteor

Meteor Reactivity for your React Native application :)
MIT License
693 stars 210 forks source link

iOS - Document removed from a published set not cleared from cache when app is re-opened from background #179

Open eawall opened 7 years ago

eawall commented 7 years ago

My Meteor collection seems to be keeping documents in the cache even after they are unpublished by the server (by changing one of their properties) when the app is re-opened from the background after locking the screen.

  1. Open the app
  2. Add a new document which becomes published
  3. Minimize the app/lock the screen
  4. Modify the first document so it becomes unpublished and add a new one that is published
  5. Re-open the app

If I follow these steps I end up with both documents being returned by my collection().find call, despite the first document no longer being published by the server.

It looks like this is probably an issue with minimongo-cache not properly clearing out data, but I thought it would be best to post the issue here first.

Also, I haven't been able to reproduce the same issue on Android.

eawall commented 7 years ago

Okay, after digging around in the code, this is what I found.

The issue is occurring on both iOS and Android, when you lose your DDP connection. I only noticed it on iOS because the connection is killed every time you lock the screen, unlike on Android where it is kept alive in the background.

When you reconnect, all of the documents currently being published by the server are sent to react-native-meteor client. An upsert is done, your documents are added/merged in their respective minimongo collection, and everything is wonderful.

However, if any documents are removed from the publication while you are offline, there is no corresponding 'removed' event that can be fire when you reconnect. So, you end up with an unpublished document stuck in your minimongo collection until memory is cleared.

The only thing I could think of doing was clearing out all documents from each collection in minimongo when you establish a DDP connection.

// react-native-meteor/src/Meteor.js:97
Data.ddp.on("connected", ()=>{
  // Begin changes
  if (Data.db && Data.db.collections) {
    // Loop through all the collection names and remove() all documents from each one
    for (var collection in Data.db.collections)
      Data.db[collection].remove({});
  }
// end changes

  Data.notify('change');

  console.info("Connected to DDP server.");
  this._loadInitialUser().then(() => {
    this._subscriptionsRestart();
  });
});

If this is just a terrible way of handling this situation, someone please please let me know. But I absolutely need to be rid of those old documents getting stuck in the cache and I can't figure out any alternative in the time I have.

Thanks!

fakenickels commented 7 years ago

This is a nice finding @eawall! Gonna do some look on this when I have some time /cc @noris666

eawall commented 7 years ago

I've done some tinkering around with this over the holidays, and I think that I have something close to a working solution.

The idea is to keep a map of collections/document ids that pass through Data.ddp.on("added") when reconnecting. Then, when all subscriptions have had their "ready" callbacks executed after reconnecting, you iterate through the minimongo collections and remove any document that ISN'T in the map. This prevents a lot of screen-flickering/re-rending when you reconnect.

I'll post a snippet of my changes to Meteor.js below. If one of the contributors feels this is an appropriate solution, I'll submit a pull request.

//...
_subscriptionsRestart() {

  for(var i in Data.subscriptions) {
    const sub = Data.subscriptions[i];
    Data.ddp.unsub(sub.subIdRemember);
    sub.subIdRemember = Data.ddp.sub(sub.name, sub.params);
    sub.resubscribed = false; // set resubscribed to false, to keep track of which subs have had 'ready' called
  }

},
_collectionIdsHash: {}, // the map to store collections/document ids

  //...

  Data.ddp.on("disconnected", ()=>{
    this._collectionIdsHash = {}; // empty out the map on disconnect, because all subs will be restarted on reconnect

    //...

  });

  Data.ddp.on("added", message => {
    if(!Data.db[message.collection]) {
      Data.db.addCollection(message.collection)
    }
    Data.db[message.collection].upsert({_id: message.id, ...message.fields});

    // add the collection/document id to the map
    if (!this._collectionIdsHash[message.collection]) {
      this._collectionIdsHash[message.collection] = [];
    }
    if (this._collectionIdsHash[message.collection].indexOf(message.id) < 0) {
      this._collectionIdsHash[message.collection].push(message.id);
    }
  });

  Data.ddp.on("ready", message => {
    const idsMap = new Map();
    for(var i in Data.subscriptions) {
      const sub = Data.subscriptions[i];
      idsMap.set(sub.subIdRemember, sub.id);
    }
    for(var i in message.subs) {
      const subId = idsMap.get(message.subs[i]);
      if(subId){
        const sub = Data.subscriptions[subId];
        sub.ready = true;
        sub.readyDeps.changed();
        sub.readyCallback && sub.readyCallback();
        sub.resubscribed = true; // set this subscription as resubscribed
      }
    }

    // iterate through all the subscriptions to see if they've all been "re-readied"
    var allResubscribed = true;

    for (var sub in Data.subscriptions) {
      if (Data.subscriptions[sub].resubscribed == false) allResubscribed = false
    }

    // if every subscription has finished, the collection/document ids map should contain the ids of every subscribed document.
    // therefore, we are able to remove all documents from minimongo that do not appear in the map
    if (allResubscribed) {
      for (var collection in this._collectionIdsHash) {
        if (this._collectionIdsHash[collection].length && Data.db && Data.db.collections && Data.db[collection]) {
          Data.db[collection].remove({
            _id: {
              $nin: this._collectionIdsHash[collection]
            }
          });
        }
      }
    }
  });

  //...

  Data.ddp.on("removed", message => {
    Data.db[message.collection] && Data.db[message.collection].del(message.id);

    // remove the document id from the map
    if (this._collectionIdsHash[message.collection] && this._collectionIdsHash[message.collection].indexOf(message.id) > -1) {
      this._collectionIdsHash[message.collection].splice(this._collectionIdsHash[message.collection].indexOf(message.id), 1);
    } 
  });
2ducanhpham commented 7 years ago

@eawall you can create PR for fix it

ujwal-setlur commented 7 years ago

Was a PR ever created for this? I am seeing this issue as well.

ujwal-setlur commented 7 years ago

@eawall I believe your earlier solution of clearing all collections on connect is actually the correct and more robust one. Here is what I ran into with your second solution:

I believe your original solution is actually the correct one!