Skip to content

Commit

Permalink
Fetch conversations once, clean up ConversationController API (#1420)
Browse files Browse the repository at this point in the history 
* Fetch conversations once, clean up ConversationController API

Race conditions around re-fetching have caused some problems recently,
so this removes the need to re-fetch conversations. They are fetched
once or saved once, and that is it. All interaction goes through the
ConversationController, which is the central source of truth.

We have two rules for Conversations:

1. If a conversation is in the ConversationController it doesn't need
   to be fetched, but its initial fetch/save might be in progress. You
   can wait for that fetch/save with conversation.initialPromise.
2. If a conversation is not already in the ConversationController, it's
   not yet in the database. It needs to be added to the
   ConversationController and saved to the database.

FREEBIE

* Remove Conversation.fetch() call in Message.handleDataMessage()

FREEBIE

* ConversationController.API cleanup: Fix two missing spots

FREEBIE
  • Loading branch information
@scottnonnenberg
scottnonnenberg committed Sep 1, 2017
1 parent 51cd28b commit d8ce198
Show file tree
Hide file tree
Showing 10 changed files with 260 additions and 278 deletions.
38 changes: 30 additions & 8 deletions js/background.js
View file
Expand Up @@ -193,7 +193,7 @@
return;
}

return ConversationController.findOrCreateById(id, 'private')
return ConversationController.getOrCreateAndWait(id, 'private')
.then(function(conversation) {
return new Promise(function(resolve, reject) {
conversation.save({
Expand Down Expand Up @@ -229,7 +229,7 @@
var details = ev.groupDetails;
var id = details.id;

return ConversationController.findOrCreateById(id, 'group').then(function(conversation) {
return ConversationController.getOrCreateAndWait(id, 'group').then(function(conversation) {
var updates = {
name: details.name,
members: details.members,
Expand Down Expand Up @@ -258,8 +258,19 @@
return;
}

return message.handleDataMessage(data.message, ev.confirm, {
initialLoadComplete: initialLoadComplete
var type, id;
if (data.message.group) {
type = 'group';
id = data.message.group.id;
} else {
type = 'private';
id = data.source;
}

return ConversationController.getOrCreateAndWait(id, type).then(function() {
return message.handleDataMessage(data.message, ev.confirm, {
initialLoadComplete: initialLoadComplete
});
});
});
}
Expand All @@ -286,8 +297,19 @@
return;
}

return message.handleDataMessage(data.message, ev.confirm, {
initialLoadComplete: initialLoadComplete
var type, id;
if (data.message.group) {
type = 'group';
id = data.message.group.id;
} else {
type = 'private';
id = data.destination;
}

return ConversationController.getOrCreateAndWait(id, type).then(function() {
return message.handleDataMessage(data.message, ev.confirm, {
initialLoadComplete: initialLoadComplete
});
});
});
}
Expand Down Expand Up @@ -374,7 +396,7 @@

return message.saveErrors(error).then(function() {
var id = message.get('conversationId');
return ConversationController.findOrCreateById(id, 'private').then(function(conversation) {
return ConversationController.getOrCreateAndWait(id, 'private').then(function(conversation) {
conversation.set({
active_at: Date.now(),
unreadCount: conversation.get('unreadCount') + 1
Expand Down Expand Up @@ -455,7 +477,7 @@
console.log('got verified sync for', number, state,
ev.viaContactSync ? 'via contact sync' : '');

return ConversationController.findOrCreateById(number, 'private').then(function(contact) {
return ConversationController.getOrCreateAndWait(number, 'private').then(function(contact) {
var options = {
viaSyncMessage: true,
viaContactSync: ev.viaContactSync,
Expand Down
55 changes: 32 additions & 23 deletions js/conversation_controller.js
View file
Expand Up @@ -85,37 +85,46 @@
get: function(id) {
return conversations.get(id);
},
add: function(attrs) {
return conversations.add(attrs, {merge: true});
createTemporary: function(attributes) {
return conversations.add(attributes);
},
create: function(attrs) {
if (typeof attrs !== 'object') {
throw new Error('ConversationController.create requires an object, got', attrs);
getOrCreate: function(id, type) {
var conversation = conversations.get(id);
if (conversation) {
return conversation;
}
var conversation = conversations.add(attrs, {merge: true});
return conversation;
},
findOrCreateById: function(id, type) {
var conversation = conversations.add({

conversation = conversations.add({
id: id,
type: type
});
return new Promise(function(resolve, reject) {
conversation.fetch().then(function() {
conversation.initialPromise = new Promise(function(resolve, reject) {
var deferred = conversation.save();

if (!deferred) {
console.log('Conversation save failed! ', id, type);
return reject(new Error('getOrCreate: Conversation save failed'));
}

deferred.then(function() {
resolve(conversation);
}, function() {
var deferred = conversation.save();
}, reject);
});

if (!deferred) {
console.log('Conversation save failed! ', id, type);
return reject(new Error('findOrCreateById: Conversation save failed'));
}
return conversation;
},
getOrCreateAndWait: function(id, type) {
var conversation = this.getOrCreate(id, type);

deferred.then(function() {
resolve(conversation);
}, reject);
if (conversation) {
return conversation.initialPromise.then(function() {
return conversation;
});
});
}

return Promise.reject(
new Error('getOrCreateAndWait: did not get conversation')
);
},
getAllGroupsInvolvingId: function(id) {
var groups = new Whisper.GroupCollection();
Expand All @@ -126,7 +135,7 @@
});
},
updateInbox: function() {
return conversations.fetchActive();
return conversations.fetch();
}
};
})();
12 changes: 6 additions & 6 deletions js/keychange_listener.js
View file
Expand Up @@ -13,13 +13,13 @@
}

signalProtocolStore.on('keychange', function(id) {
var conversation = ConversationController.add({id: id});
conversation.fetch().then(function() {
ConversationController.getOrCreateAndWait(id, 'private').then(function(conversation) {
conversation.addKeyChange(id);
});
ConversationController.getAllGroupsInvolvingId(id).then(function(groups) {
_.forEach(groups, function(group) {
group.addKeyChange(id);

ConversationController.getAllGroupsInvolvingId(id).then(function(groups) {
_.forEach(groups, function(group) {
group.addKeyChange(id);
});
});
});
});
Expand Down
81 changes: 27 additions & 54 deletions js/models/conversations.js
View file
Expand Up @@ -59,6 +59,10 @@
this.ourNumber = textsecure.storage.user.getNumber();
this.verifiedEnum = textsecure.storage.protocol.VerifiedStatus;

// This may be overridden by ConversationController.getOrCreate, and signify
// our first save to the database. Or first fetch from the database.
this.initialPromise = Promise.resolve();

this.contactCollection = new Backbone.Collection();
this.messageCollection = new Whisper.MessageCollection([], {
conversation: this
Expand Down Expand Up @@ -87,7 +91,7 @@
if (this.isPrivate()) {
return Promise.all([
this.safeGetVerified(),
this.safeFetch()
this.initialPromise,
]).then(function(results) {
var trust = results[0];
// we don't return here because we don't need to wait for this to finish
Expand All @@ -103,12 +107,6 @@
}.bind(this)).then(this.onMemberVerifiedChange.bind(this));
}
},
safeFetch: function() {
// new Promise necessary because a fetch will fail if convo not in db yet
return new Promise(function(resolve) {
this.fetch().always(resolve);
}.bind(this));
},
setVerifiedDefault: function(options) {
var DEFAULT = this.verifiedEnum.DEFAULT;
return this.queueJob(function() {
Expand Down Expand Up @@ -801,28 +799,23 @@
return _.contains(this.get('members'), number);
},
fetchContacts: function(options) {
return new Promise(function(resolve) {
if (this.isPrivate()) {
this.contactCollection.reset([this]);
resolve();
} else {
var promises = [];
var members = this.get('members') || [];

this.contactCollection.reset(
members.map(function(number) {
var c = ConversationController.create({
id : number,
type : 'private'
});
this.listenTo(c, 'change:verified', this.onMemberVerifiedChange);
promises.push(c.safeFetch());
return c;
}.bind(this))
);
resolve(Promise.all(promises));
}
}.bind(this));
if (this.isPrivate()) {
this.contactCollection.reset([this]);
return Promise.resolve();
} else {
var members = this.get('members') || [];
var promises = members.map(function(number) {
return ConversationController.getOrCreateAndWait(number, 'private');
});

return Promise.all(promises).then(function(contacts) {
_.forEach(contacts, function(contact) {
this.listenTo(contact, 'change:verified', this.onMemberVerifiedChange);
}.bind(this));

this.contactCollection.reset(contacts);
}.bind(this));
}
},

destroyMessages: function() {
Expand Down Expand Up @@ -956,14 +949,11 @@
}

window.drawAttention();
var sender = ConversationController.create({
id: message.get('source'), type: 'private'
});
var conversationId = this.id;

return new Promise(function(resolve, reject) {
sender.fetch().then(function() {
sender.getNotificationIcon().then(function(iconUrl) {
ConversationController.getOrCreateAndWait(message.get('source'), 'private')
.then(function(sender) {
return sender.getNotificationIcon().then(function(iconUrl) {
console.log('adding notification');
Whisper.Notifications.add({
title : sender.getTitle(),
Expand All @@ -973,11 +963,8 @@
conversationId : conversationId,
messageId : message.id
});

return resolve();
}, reject);
}, reject);
});
});
});
},
hashCode: function() {
if (this.hash === undefined) {
Expand Down Expand Up @@ -1054,20 +1041,6 @@
}
}).always(resolve);
}.bind(this));
},

fetchActive: function() {
// Ensures all active conversations are included in this collection,
// and updates their attributes, but removes nothing.
return this.fetch({
index: {
name: 'inbox', // 'inbox' index on active_at
order: 'desc' // ORDER timestamp DESC
// TODO pagination/infinite scroll
// limit: 10, offset: page*10,
},
remove: false
});
}
});

Expand Down

0 comments on commit d8ce198

Please sign in to comment.