From 74cb808763280e4453be9f5fd5e330ea6bfc8db1 Mon Sep 17 00:00:00 2001 From: Scott Nonnenberg Date: Mon, 25 Mar 2019 18:10:30 -0700 Subject: [PATCH] New MessageController as the single place for in-memory messages --- background.html | 1 + js/delivery_receipts.js | 18 ++++++--- js/expiring_messages.js | 17 +++++--- js/message_controller.js | 65 ++++++++++++++++++++++++++++++ js/models/conversations.js | 41 +++++++++---------- js/models/messages.js | 34 ++++++++++------ js/modules/attachment_downloads.js | 52 ++++-------------------- js/read_receipts.js | 16 +++++++- js/read_syncs.js | 17 +++++--- js/views/conversation_view.js | 12 +++--- test/index.html | 1 + 11 files changed, 169 insertions(+), 105 deletions(-) create mode 100644 js/message_controller.js diff --git a/background.html b/background.html index 2e0f6014e..d8ffd6392 100644 --- a/background.html +++ b/background.html @@ -492,6 +492,7 @@ + diff --git a/js/delivery_receipts.js b/js/delivery_receipts.js index a88aa67a2..ea52a6822 100644 --- a/js/delivery_receipts.js +++ b/js/delivery_receipts.js @@ -1,7 +1,10 @@ -/* global Backbone: false */ -/* global Whisper: false */ -/* global ConversationController: false */ -/* global _: false */ +/* global + Backbone, + Whisper, + ConversationController, + MessageController, + _ +*/ /* eslint-disable more/no-then */ @@ -45,10 +48,15 @@ const ids = groups.pluck('id'); ids.push(source); - return messages.find( + const target = messages.find( item => !item.isIncoming() && _.contains(ids, item.get('conversationId')) ); + if (!target) { + return null; + } + + return MessageController.register(target.id, target); }, async onReceipt(receipt) { try { diff --git a/js/expiring_messages.js b/js/expiring_messages.js index f187a92aa..365c03fc5 100644 --- a/js/expiring_messages.js +++ b/js/expiring_messages.js @@ -1,8 +1,11 @@ -/* global _: false */ -/* global Backbone: false */ -/* global i18n: false */ -/* global moment: false */ -/* global Whisper: false */ +/* global + _, + Backbone, + i18n, + MessageController, + moment, + Whisper +*/ // eslint-disable-next-line func-names (function() { @@ -18,7 +21,9 @@ }); await Promise.all( - messages.map(async message => { + messages.map(async fromDB => { + const message = MessageController.register(fromDB.id, fromDB); + window.log.info('Message expired', { sentAt: message.get('sent_at'), }); diff --git a/js/message_controller.js b/js/message_controller.js new file mode 100644 index 000000000..18d58cde9 --- /dev/null +++ b/js/message_controller.js @@ -0,0 +1,65 @@ +// eslint-disable-next-line func-names +(function() { + 'use strict'; + + window.Whisper = window.Whisper || {}; + + const messageLookup = Object.create(null); + + const SECOND = 1000; + const MINUTE = SECOND * 60; + const FIVE_MINUTES = MINUTE * 5; + const HOUR = MINUTE * 60; + + function register(id, message) { + const existing = messageLookup[id]; + if (existing) { + messageLookup[id] = { + message: existing.message, + timestamp: Date.now(), + }; + return existing.message; + } + + messageLookup[id] = { + message, + timestamp: Date.now(), + }; + + return message; + } + + function unregister(id) { + delete messageLookup[id]; + } + + function cleanup() { + const messages = Object.values(messageLookup); + const now = Date.now(); + + for (let i = 0, max = messages.length; i < max; i += 1) { + const { message, timestamp } = messages[i]; + const conversation = message.getConversation(); + + if ( + now - timestamp > FIVE_MINUTES && + (!conversation || !conversation.messageCollection.length) + ) { + delete messageLookup[message.id]; + } + } + } + + function _get() { + return messageLookup; + } + + setInterval(cleanup, HOUR); + + window.MessageController = { + register, + unregister, + cleanup, + _get, + }; +})(); diff --git a/js/models/conversations.js b/js/models/conversations.js index 1e7552a7d..ed7fc66d1 100644 --- a/js/models/conversations.js +++ b/js/models/conversations.js @@ -4,6 +4,7 @@ Backbone, libphonenumber, ConversationController, + MessageController, libsignal, storage, textsecure, @@ -673,14 +674,6 @@ }, async onReadMessage(message, readAt) { - const existing = this.messageCollection.get(message.id); - if (existing) { - const fetched = await window.Signal.Data.getMessageById(existing.id, { - Message: Whisper.Message, - }); - existing.merge(fetched); - } - // We mark as read everything older than this message - to clean up old stuff // still marked unread in the database. If the user generally doesn't read in // the desktop app, so the desktop app only gets read syncs, we can very @@ -883,10 +876,23 @@ recipients, }); - const message = this.addSingleMessage(messageWithSchema); + if (this.isPrivate()) { + messageWithSchema.destination = destination; + } + const attributes = { + ...messageWithSchema, + id: window.getGuid(), + }; + + const model = this.addSingleMessage(attributes); + const message = MessageController.register(model.id, model); + await window.Signal.Data.saveMessage(message.attributes, { + forceSave: true, + Message: Whisper.Message, + }); this.set({ - lastMessage: message.getNotificationText(), + lastMessage: model.getNotificationText(), lastMessageStatus: 'sending', active_at: now, timestamp: now, @@ -896,15 +902,6 @@ Conversation: Whisper.Conversation, }); - if (this.isPrivate()) { - message.set({ destination }); - } - - const id = await window.Signal.Data.saveMessage(message.attributes, { - Message: Whisper.Message, - }); - message.set({ id }); - // We're offline! if (!textsecure.messaging) { const errors = this.contactCollection.map(contact => { @@ -1424,11 +1421,9 @@ let read = await Promise.all( _.map(oldUnread, async providedM => { - let m = providedM; + const m = MessageController.register(providedM.id, providedM); - if (this.messageCollection.get(m.id)) { - m = this.messageCollection.get(m.id); - } else { + if (!this.messageCollection.get(m.id)) { window.log.warn( 'Marked a message as read in the database, but ' + 'it was not in messageCollection.' diff --git a/js/models/messages.js b/js/models/messages.js index 1bae4fc30..00626e523 100644 --- a/js/models/messages.js +++ b/js/models/messages.js @@ -1,13 +1,16 @@ -/* global _: false */ -/* global Backbone: false */ -/* global storage: false */ -/* global filesize: false */ -/* global ConversationController: false */ -/* global getAccountManager: false */ -/* global i18n: false */ -/* global Signal: false */ -/* global textsecure: false */ -/* global Whisper: false */ +/* global + _, + Backbone, + storage, + filesize, + ConversationController, + MessageController, + getAccountManager, + i18n, + Signal, + textsecure, + Whisper +*/ /* eslint-disable more/no-then */ @@ -261,6 +264,7 @@ this.cleanup(); }, async cleanup() { + MessageController.unregister(this.id); this.unload(); await deleteExternalMessageFiles(this.attributes); }, @@ -1394,17 +1398,18 @@ const collection = await window.Signal.Data.getMessagesBySentAt(id, { MessageCollection: Whisper.MessageCollection, }); - const queryMessage = collection.find(item => { + const found = collection.find(item => { const messageAuthor = item.getContact(); return messageAuthor && author === messageAuthor.id; }); - if (!queryMessage) { + if (!found) { quote.referencedMessageNotFound = true; return message; } + const queryMessage = MessageController.register(found.id, found); quote.text = queryMessage.get('body'); if (firstAttachment) { firstAttachment.thumbnail = null; @@ -1730,6 +1735,7 @@ Message: Whisper.Message, }); message.set({ id }); + MessageController.register(message.id, message); // Note that this can save the message again, if jobs were queued. We need to // call it after we have an id for this message, because the jobs refer back @@ -1933,7 +1939,9 @@ } ); - const models = messages.filter(message => Boolean(message.id)); + const models = messages + .filter(message => Boolean(message.id)) + .map(message => MessageController.register(message.id, message)); const eliminated = messages.length - models.length; if (eliminated > 0) { window.log.warn( diff --git a/js/modules/attachment_downloads.js b/js/modules/attachment_downloads.js index d330c8653..e4356dc83 100644 --- a/js/modules/attachment_downloads.js +++ b/js/modules/attachment_downloads.js @@ -1,4 +1,4 @@ -/* global Whisper, Signal, setTimeout, clearTimeout */ +/* global Whisper, Signal, setTimeout, clearTimeout, MessageController */ const { isFunction, isNumber, omit } = require('lodash'); const getGuid = require('uuid/v4'); @@ -37,7 +37,6 @@ let timeout; let getMessageReceiver; let logger; const _activeAttachmentDownloadJobs = {}; -const _messageCache = {}; async function start(options = {}) { ({ getMessageReceiver, logger } = options); @@ -149,12 +148,15 @@ async function _runJob(job) { ); } - message = await _getMessage(messageId); - if (!message) { + const found = await getMessageById(messageId, { + Message: Whisper.Message, + }); + if (!found) { logger.error('_runJob: Source message not found, deleting job'); - await _finishJob(message, id); + await _finishJob(null, id); return; } + message = MessageController.register(found.id, found); const pending = true; await setAttachmentDownloadJobPending(id, pending); @@ -232,46 +234,6 @@ async function _runJob(job) { } } -async function _getMessage(id) { - let item = _messageCache[id]; - if (item) { - const fiveMinutesAgo = Date.now() - 5 * MINUTE; - if (item.timestamp >= fiveMinutesAgo) { - return item.message; - } - - delete _messageCache[id]; - } - - let message = await getMessageById(id, { - Message: Whisper.Message, - }); - if (!message) { - return message; - } - - // Once more, checking for race conditions - item = _messageCache[id]; - if (item) { - const fiveMinutesAgo = Date.now() - 5 * MINUTE; - if (item.timestamp >= fiveMinutesAgo) { - return item.message; - } - } - - const conversation = message.getConversation(); - if (conversation && conversation.messageCollection.get(id)) { - message = conversation.get(id); - } - - _messageCache[id] = { - timestamp: Date.now(), - message, - }; - - return message; -} - async function _finishJob(message, id) { if (message) { await saveMessage(message.attributes, { diff --git a/js/read_receipts.js b/js/read_receipts.js index 989629939..e44380874 100644 --- a/js/read_receipts.js +++ b/js/read_receipts.js @@ -1,4 +1,11 @@ -/* global Whisper, Backbone, _, ConversationController, window */ +/* global + Whisper, + Backbone, + _, + ConversationController, + MessageController, + window +*/ /* eslint-disable more/no-then */ @@ -46,9 +53,14 @@ const ids = groups.pluck('id'); ids.push(reader); - return messages.find( + const target = messages.find( item => item.isOutgoing() && _.contains(ids, item.get('conversationId')) ); + if (!target) { + return null; + } + + return MessageController.register(target.id, target); }, async onReceipt(receipt) { try { diff --git a/js/read_syncs.js b/js/read_syncs.js index 4732445c3..932794328 100644 --- a/js/read_syncs.js +++ b/js/read_syncs.js @@ -1,4 +1,8 @@ -/* global Backbone, Whisper */ +/* global + Backbone, + Whisper, + MessageController +*/ /* eslint-disable more/no-then */ @@ -30,19 +34,19 @@ } ); - const message = messages.find( + const found = messages.find( item => item.isIncoming() && item.get('source') === receipt.get('sender') ); - const notificationForMessage = message - ? Whisper.Notifications.findWhere({ messageId: message.id }) + const notificationForMessage = found + ? Whisper.Notifications.findWhere({ messageId: found.id }) : null; const removedNotification = Whisper.Notifications.remove( notificationForMessage ); const receiptSender = receipt.get('sender'); const receiptTimestamp = receipt.get('timestamp'); - const wasMessageFound = Boolean(message); + const wasMessageFound = Boolean(found); const wasNotificationFound = Boolean(notificationForMessage); const wasNotificationRemoved = Boolean(removedNotification); window.log.info('Receive read sync:', { @@ -53,10 +57,11 @@ wasNotificationRemoved, }); - if (!message) { + if (!found) { return; } + const message = MessageController.register(found.id, found); const readAt = receipt.get('read_at'); // If message is unread, we mark it read. Otherwise, we update the expiration diff --git a/js/views/conversation_view.js b/js/views/conversation_view.js index de447df5f..f83506088 100644 --- a/js/views/conversation_view.js +++ b/js/views/conversation_view.js @@ -729,13 +729,15 @@ const collection = await window.Signal.Data.getMessagesBySentAt(id, { MessageCollection: Whisper.MessageCollection, }); - const messageFromDatabase = collection.find(item => { - const messageAuthor = item.getContact(); + const found = Boolean( + collection.find(item => { + const messageAuthor = item.getContact(); - return messageAuthor && author === messageAuthor.id; - }); + return messageAuthor && author === messageAuthor.id; + }) + ); - if (messageFromDatabase) { + if (found) { const toast = new Whisper.FoundButNotLoadedToast(); toast.$el.appendTo(this.$el); toast.render(); diff --git a/test/index.html b/test/index.html index e6646b03b..68cd0169f 100644 --- a/test/index.html +++ b/test/index.html @@ -478,6 +478,7 @@ +