From ad224822740369795a42c66c235c3f5912cca588 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Fri, 7 Oct 2022 12:22:15 +1100 Subject: [PATCH 01/10] fix: add toast on rate limit hit for reactions --- _locales/en/messages.json | 1 + .../apis/open_group_api/sogsv3/sogsV3SendReaction.ts | 4 +++- ts/session/utils/Toast.tsx | 4 ++++ ts/types/LocalizerKeys.ts | 1 + ts/util/reactions.ts | 11 ++++++----- 5 files changed, 15 insertions(+), 6 deletions(-) diff --git a/_locales/en/messages.json b/_locales/en/messages.json index e4be9f400..922e556c6 100644 --- a/_locales/en/messages.json +++ b/_locales/en/messages.json @@ -484,6 +484,7 @@ "clearAllReactions": "Are you sure you want to clear all $emoji$ ?", "expandedReactionsText": "Show Less", "reactionNotification": "Reacts to a message with $emoji$", + "rateLimitReactMessage": "Slow down! You've sent too many emoji reacts. Try again soon", "otherSingular": "$number$ other", "otherPlural": "$number$ others", "reactionPopup": "reacted with", diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts index 445219e40..5a112dca3 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts @@ -5,7 +5,7 @@ import { Action, OpenGroupReactionResponse, Reaction } from '../../../../types/R import { getEmojiDataFromNative } from '../../../../util/emoji'; import { Reactions } from '../../../../util/reactions'; import { OnionSending } from '../../../onions/onionSend'; -import { UserUtils } from '../../../utils'; +import { ToastUtils, UserUtils } from '../../../utils'; import { OpenGroupPollingUtils } from '../opengroupV2/OpenGroupPollingUtils'; import { getUsBlindedInThatServer } from './knownBlindedkeys'; import { batchGlobalIsSuccess, parseBatchGlobalStatusCode } from './sogsV3BatchPoll'; @@ -58,6 +58,8 @@ export const sendSogsReactionOnionV4 = async ( } if (Reactions.hitRateLimit()) { + ToastUtils.pushRateLimitHitReactions(); + return false; } diff --git a/ts/session/utils/Toast.tsx b/ts/session/utils/Toast.tsx index 54a1ba80f..4170dc64d 100644 --- a/ts/session/utils/Toast.tsx +++ b/ts/session/utils/Toast.tsx @@ -277,3 +277,7 @@ export function pushNoMediaUntilApproved() { export function pushMustBeApproved() { pushToastError('mustBeApproved', window.i18n('mustBeApproved')); } + +export function pushRateLimitHitReactions() { + pushToastInfo('reactRateLimit', '', window?.i18n?.('rateLimitReactMessage')); // because otherwise test fails +} diff --git a/ts/types/LocalizerKeys.ts b/ts/types/LocalizerKeys.ts index 95a12580f..f23f5fb23 100644 --- a/ts/types/LocalizerKeys.ts +++ b/ts/types/LocalizerKeys.ts @@ -87,6 +87,7 @@ export type LocalizerKeys = | 'enterNewPassword' | 'expandedReactionsText' | 'openMessageRequestInbox' + | 'rateLimitReactMessage' | 'enterPassword' | 'enterSessionIDOfRecipient' | 'join' diff --git a/ts/util/reactions.ts b/ts/util/reactions.ts index 3193ba5d3..0659c5224 100644 --- a/ts/util/reactions.ts +++ b/ts/util/reactions.ts @@ -6,7 +6,7 @@ import { getUsBlindedInThatServer, isUsAnySogsFromCache, } from '../session/apis/open_group_api/sogsv3/knownBlindedkeys'; -import { UserUtils } from '../session/utils'; +import { ToastUtils, UserUtils } from '../session/utils'; import { Action, OpenGroupReactionList, ReactionList, RecentReactions } from '../types/Reaction'; import { getRecentReactions, saveRecentReations } from '../util/storage'; @@ -17,14 +17,14 @@ const rateTimeLimit = 60 * 1000; const latestReactionTimestamps: Array = []; function hitRateLimit(): boolean { - const timestamp = Date.now(); - latestReactionTimestamps.push(timestamp); + const now = Date.now(); + latestReactionTimestamps.push(now); if (latestReactionTimestamps.length > rateCountLimit) { const firstTimestamp = latestReactionTimestamps[0]; - if (timestamp - firstTimestamp < rateTimeLimit) { + if (now - firstTimestamp < rateTimeLimit) { latestReactionTimestamps.pop(); - window.log.warn('Only 20 reactions are allowed per minute'); + window.log.warn(`Only ${rateCountLimit} reactions are allowed per minute`); return true; } else { latestReactionTimestamps.shift(); @@ -86,6 +86,7 @@ const sendMessageReaction = async (messageId: string, emoji: string) => { } if (hitRateLimit()) { + ToastUtils.pushRateLimitHitReactions(); return; } From 0cc7994c12da692af640a8a61500bfcd47a00844 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Thu, 6 Oct 2022 15:15:14 +1100 Subject: [PATCH 02/10] fix: speed up expiration/deletion of messages by batching updates in UI --- .../message-item/GenericReadableMessage.tsx | 12 +-- ts/data/data.ts | 14 ++-- ts/data/dataInit.ts | 2 +- ts/mains/main_renderer.tsx | 9 ++- ts/models/conversation.ts | 18 ++--- ts/models/message.ts | 1 - ts/node/sql.ts | 41 ++++++++-- .../apis/open_group_api/sogsv3/sogsApiV3.ts | 51 ++++++++++--- .../open_group_api/sogsv3/sogsV3BatchPoll.ts | 3 +- ts/state/ducks/conversations.ts | 57 +++++++++----- ts/util/expiringMessages.ts | 75 ++++++++++++------- 11 files changed, 191 insertions(+), 92 deletions(-) diff --git a/ts/components/conversation/message/message-item/GenericReadableMessage.tsx b/ts/components/conversation/message/message-item/GenericReadableMessage.tsx index eda7b73d1..8e54243ca 100644 --- a/ts/components/conversation/message/message-item/GenericReadableMessage.tsx +++ b/ts/components/conversation/message/message-item/GenericReadableMessage.tsx @@ -8,7 +8,7 @@ import _ from 'lodash'; import { Data } from '../../../../data/data'; import { MessageRenderingProps } from '../../../../models/messageType'; import { getConversationController } from '../../../../session/conversations'; -import { messageExpired } from '../../../../state/ducks/conversations'; +import { messagesExpired } from '../../../../state/ducks/conversations'; import { getGenericReadableMessageSelectorProps, getIsMessageSelected, @@ -68,10 +68,12 @@ function useIsExpired(props: ExpiringProps) { await Data.removeMessage(messageId); if (convoId) { dispatch( - messageExpired({ - conversationKey: convoId, - messageId, - }) + messagesExpired([ + { + conversationKey: convoId, + messageId, + }, + ]) ); const convo = getConversationController().get(convoId); convo?.updateLastMessage(); diff --git a/ts/data/data.ts b/ts/data/data.ts index b14aa9b4d..f7adbe688 100644 --- a/ts/data/data.ts +++ b/ts/data/data.ts @@ -139,7 +139,7 @@ export const Data = { saveMessage, saveMessages, removeMessage, - _removeMessages, + removeMessagesByIds, getMessageIdsFromServerIds, getMessageById, getMessageBySenderAndSentAt, @@ -390,9 +390,13 @@ async function removeMessage(id: string): Promise { } } -// Note: this method will not clean up external files, just delete from SQL -async function _removeMessages(ids: Array): Promise { - await channels.removeMessage(ids); +/** + * Note: this method will not clean up external files, just delete from SQL. + * File are cleaned up on app start if they are not linked to any messages + * + */ +async function removeMessagesByIds(ids: Array): Promise { + await channels.removeMessagesByIds(ids); } async function getMessageIdsFromServerIds( @@ -630,7 +634,7 @@ async function removeAllMessagesInConversation(conversationId: string): Promise< await Promise.all(messages.map(message => message.cleanup())); // eslint-disable-next-line no-await-in-loop - await channels.removeMessage(ids); + await channels.removeMessagesByIds(ids); } while (messages.length > 0); } diff --git a/ts/data/dataInit.ts b/ts/data/dataInit.ts index 1bd0f40b7..f838adfcc 100644 --- a/ts/data/dataInit.ts +++ b/ts/data/dataInit.ts @@ -38,7 +38,7 @@ const channelsToMake = new Set([ 'saveSeenMessageHashes', 'saveMessages', 'removeMessage', - '_removeMessages', + 'removeMessagesByIds', 'getUnreadByConversation', 'markAllAsReadByConversationNoExpiration', 'getUnreadCountByConversation', diff --git a/ts/mains/main_renderer.tsx b/ts/mains/main_renderer.tsx index 367613e28..3fbf89cea 100644 --- a/ts/mains/main_renderer.tsx +++ b/ts/mains/main_renderer.tsx @@ -212,8 +212,10 @@ async function start() { ); window.log.info(`Cleanup: Found ${messagesForCleanup.length} messages for cleanup`); + + const idsToCleanUp: Array = []; await Promise.all( - messagesForCleanup.map(async (message: MessageModel) => { + messagesForCleanup.map((message: MessageModel) => { const sentAt = message.get('sent_at'); if (message.hasErrors()) { @@ -221,9 +223,12 @@ async function start() { } window.log.info(`Cleanup: Deleting unsent message ${sentAt}`); - await Data.removeMessage(message.id); + idsToCleanUp.push(message.id); }) ); + if (idsToCleanUp.length) { + await Data.removeMessagesByIds(idsToCleanUp); + } window.log.info('Cleanup: complete'); window.log.info('listening for registration events'); diff --git a/ts/models/conversation.ts b/ts/models/conversation.ts index fe3942186..3795f5720 100644 --- a/ts/models/conversation.ts +++ b/ts/models/conversation.ts @@ -248,12 +248,6 @@ export class ConversationModel extends Backbone.Model { await deleteExternalFilesOfConversation(this.attributes); } - public async onExpired(_message: MessageModel) { - await this.updateLastMessage(); - - // removeMessage(); - } - public getGroupAdmins(): Array { const groupAdmins = this.get('groupAdmins'); @@ -1681,15 +1675,17 @@ export class ConversationModel extends Backbone.Model { return this.get('type') === ConversationTypeEnum.GROUP; } - public async removeMessage(messageId: any) { + public async removeMessage(messageId: string) { await Data.removeMessage(messageId); this.updateLastMessage(); window.inboxStore?.dispatch( - conversationActions.messageDeleted({ - conversationKey: this.id, - messageId, - }) + conversationActions.messagesDeleted([ + { + conversationKey: this.id, + messageId, + }, + ]) ); } diff --git a/ts/models/message.ts b/ts/models/message.ts index 9671b95d6..2096133ee 100644 --- a/ts/models/message.ts +++ b/ts/models/message.ts @@ -125,7 +125,6 @@ export class MessageModel extends Backbone.Model { throw new Error('A message always needs to have an conversationId.'); } - // this.on('expired', this.onExpired); if (!attributes.skipTimerInit) { void this.setToExpire(); } diff --git a/ts/node/sql.ts b/ts/node/sql.ts index f7270e7f7..d7580d63e 100644 --- a/ts/node/sql.ts +++ b/ts/node/sql.ts @@ -945,21 +945,44 @@ function saveMessages(arrayOfMessages: Array) { } function removeMessage(id: string, instance?: BetterSqlite3.Database) { - if (!Array.isArray(id)) { - assertGlobalInstanceOrInstance(instance) - .prepare(`DELETE FROM ${MESSAGES_TABLE} WHERE id = $id;`) - .run({ id }); + if (!isString(id)) { + throw new Error('removeMessage: only takes single message to delete!'); + return; } - if (!id.length) { - throw new Error('removeMessages: No ids to delete!'); + assertGlobalInstanceOrInstance(instance) + .prepare(`DELETE FROM ${MESSAGES_TABLE} WHERE id = $id;`) + .run({ id }); +} + +function removeMessagesByIds(ids: Array, instance?: BetterSqlite3.Database) { + if (!Array.isArray(ids)) { + throw new Error('removeMessagesByIds only allowed an array of strings'); + } + + if (!ids.length) { + throw new Error('removeMessagesByIds: No ids to delete!'); } // Our node interface doesn't seem to allow you to replace one single ? with an array assertGlobalInstanceOrInstance(instance) - .prepare(`DELETE FROM ${MESSAGES_TABLE} WHERE id IN ( ${id.map(() => '?').join(', ')} );`) - .run(id); + .prepare(`DELETE FROM ${MESSAGES_TABLE} WHERE id IN ( ${ids.map(() => '?').join(', ')} );`) + .run(ids); +} + +function removeAllMessagesInConversation( + conversationId: string, + instance?: BetterSqlite3.Database +) { + if (!conversationId) { + return; + } + + // Our node interface doesn't seem to allow you to replace one single ? with an array + assertGlobalInstanceOrInstance(instance) + .prepare(`DELETE FROM ${MESSAGES_TABLE} WHERE conversationId = $conversationId`) + .run({ conversationId }); } function getMessageIdsFromServerIds(serverIds: Array, conversationId: string) { @@ -2435,6 +2458,8 @@ export const sqlNode = { updateLastHash, saveMessages, removeMessage, + removeMessagesByIds, + removeAllMessagesInConversation, getUnreadByConversation, markAllAsReadByConversationNoExpiration, getUnreadCountByConversation, diff --git a/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts b/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts index dbaa248de..79c1880f3 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts @@ -1,4 +1,4 @@ -import _, { compact, isArray, isNumber, isObject, pick } from 'lodash'; +import _, { compact, isArray, isEmpty, isNumber, isObject, pick } from 'lodash'; import { OpenGroupData } from '../../../../data/opengroups'; import { handleOpenGroupV4Message } from '../../../../receiver/opengroup'; import { OpenGroupRequestCommonType } from '../opengroupV2/ApiUtil'; @@ -35,6 +35,7 @@ import { ConversationTypeEnum } from '../../../../models/conversationAttributes' import { createSwarmMessageSentFromUs } from '../../../../models/messageFactory'; import { Data } from '../../../../data/data'; import { processMessagesUsingCache } from './sogsV3MutationCache'; +import { destroyMessagesAndUpdateRedux } from '../../../../util/expiringMessages'; /** * Get the convo matching those criteria and make sure it is an opengroup convo, or return null. @@ -154,6 +155,7 @@ async function filterOutMessagesInvalidSignature( return signaturesValidMessages; } +let totalDeletedMessages = 0; const handleSogsV3DeletedMessages = async ( messages: Array, serverUrl: string, @@ -164,29 +166,38 @@ const handleSogsV3DeletedMessages = async ( if (!deletions.length) { return messages; } + totalDeletedMessages += deletions.length; + console.warn( + JSON.stringify({ + totalDeletedMessages, + }) + ); const allIdsRemoved = deletions.map(m => m.id); try { const convoId = getOpenGroupV2ConversationId(serverUrl, roomId); const convo = getConversationController().get(convoId); const messageIds = await Data.getMessageIdsFromServerIds(allIdsRemoved, convo.id); - // we shouldn't get too many messages to delete at a time, so no need to add a function to remove multiple messages for now - - await Promise.all( - (messageIds || []).map(async id => { - if (convo) { - await convo.removeMessage(id); - } - await Data.removeMessage(id); - }) - ); + if (messageIds && messageIds.length) { + await destroyMessagesAndUpdateRedux( + messageIds.map(messageId => ({ + conversationKey: convoId, + messageId, + })) + ); + } } catch (e) { window?.log?.warn('handleDeletions failed:', e); } return exceptDeletion; }; -// tslint:disable-next-line: cyclomatic-complexity +// tslint:disable-next-line: one-variable-per-declaration +let totalEmptyReactions = 0, + totalMessagesWithResolvedBlindedIdsIfFound = 0, + totalMessageReactions = 0; + +// tslint:disable-next-line: max-func-body-length cyclomatic-complexity const handleMessagesResponseV4 = async ( messages: Array, serverUrl: string, @@ -284,6 +295,9 @@ const handleMessagesResponseV4 = async ( const incomingMessageSeqNo = compact(messages.map(n => n.seqno)); const maxNewMessageSeqNo = Math.max(...incomingMessageSeqNo); + + totalMessagesWithResolvedBlindedIdsIfFound += messagesWithResolvedBlindedIdsIfFound.length; + for (let index = 0; index < messagesWithResolvedBlindedIdsIfFound.length; index++) { const msgToHandle = messagesWithResolvedBlindedIdsIfFound[index]; try { @@ -309,6 +323,18 @@ const handleMessagesResponseV4 = async ( await OpenGroupData.saveV2OpenGroupRoom(roomInfosRefreshed); const messagesWithReactions = messages.filter(m => m.reactions !== undefined); + const messagesWithEmptyReactions = messagesWithReactions.filter(m => isEmpty(m.reactions)); + + totalMessageReactions += messagesWithReactions.length; + totalEmptyReactions += messagesWithEmptyReactions.length; + console.warn( + JSON.stringify({ + totalMessagesWithResolvedBlindedIdsIfFound, + totalMessageReactions, + totalEmptyReactions, + }) + ); + if (messagesWithReactions.length > 0) { const conversationId = getOpenGroupV2ConversationId(serverUrl, roomId); const groupConvo = getConversationController().get(conversationId); @@ -526,6 +552,7 @@ export const handleBatchPollResults = async ( break; case 'pollInfo': await handlePollInfoResponse(subResponse.code, subResponse.body, serverUrl); + break; case 'inbox': await handleInboxOutboxMessages(subResponse.body, serverUrl, false); diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3BatchPoll.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3BatchPoll.ts index 7461b16f4..cef51e209 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3BatchPoll.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3BatchPoll.ts @@ -241,7 +241,8 @@ const makeBatchRequestPayload = ( method: 'GET', path: isNumber(options.messages.sinceSeqNo) ? `/room/${options.messages.roomId}/messages/since/${options.messages.sinceSeqNo}?t=r&reactors=${Reactions.SOGSReactorsFetchCount}` - : `/room/${options.messages.roomId}/messages/recent?reactors=${Reactions.SOGSReactorsFetchCount}`, + : // : `/room/${options.messages.roomId}/messages/since/180000?t=r&reactors=${Reactions.SOGSReactorsFetchCount}`, + `/room/${options.messages.roomId}/messages/recent?reactors=${Reactions.SOGSReactorsFetchCount}`, }; } break; diff --git a/ts/state/ducks/conversations.ts b/ts/state/ducks/conversations.ts index 3d4614d4b..e6ce8caee 100644 --- a/ts/state/ducks/conversations.ts +++ b/ts/state/ducks/conversations.ts @@ -504,12 +504,12 @@ function handleMessagesChangedOrAdded( function handleMessageExpiredOrDeleted( state: ConversationsStateType, - action: PayloadAction<{ + payload: { messageId: string; conversationKey: string; - }> -): ConversationsStateType { - const { conversationKey, messageId } = action.payload; + } +) { + const { conversationKey, messageId } = payload; if (conversationKey === state.selectedConversation) { // search if we find this message id. // we might have not loaded yet, so this case might not happen @@ -539,6 +539,23 @@ function handleMessageExpiredOrDeleted( return state; } +function handleMessagesExpiredOrDeleted( + state: ConversationsStateType, + action: PayloadAction< + Array<{ + messageId: string; + conversationKey: string; + }> + > +): ConversationsStateType { + action.payload.forEach(element => { + // tslint:disable-next-line: no-parameter-reassignment + state = handleMessageExpiredOrDeleted(state, element); + }); + + return state; +} + function handleConversationReset(state: ConversationsStateType, action: PayloadAction) { const conversationKey = action.payload; if (conversationKey === state.selectedConversation) { @@ -670,24 +687,28 @@ const conversationsSlice = createSlice({ return handleMessagesChangedOrAdded(state, action.payload); }, - messageExpired( + messagesExpired( state: ConversationsStateType, - action: PayloadAction<{ - messageId: string; - conversationKey: string; - }> + action: PayloadAction< + Array<{ + messageId: string; + conversationKey: string; + }> + > ) { - return handleMessageExpiredOrDeleted(state, action); + return handleMessagesExpiredOrDeleted(state, action); }, - messageDeleted( + messagesDeleted( state: ConversationsStateType, - action: PayloadAction<{ - messageId: string; - conversationKey: string; - }> + action: PayloadAction< + Array<{ + messageId: string; + conversationKey: string; + }> + > ) { - return handleMessageExpiredOrDeleted(state, action); + return handleMessagesExpiredOrDeleted(state, action); }, conversationReset(state: ConversationsStateType, action: PayloadAction) { @@ -973,8 +994,8 @@ export const { conversationsChanged, conversationRemoved, removeAllConversations, - messageExpired, - messageDeleted, + messagesExpired, + messagesDeleted, conversationReset, messagesChanged, resetOldTopMessageId, diff --git a/ts/util/expiringMessages.ts b/ts/util/expiringMessages.ts index 147e0f51d..31e1381d5 100644 --- a/ts/util/expiringMessages.ts +++ b/ts/util/expiringMessages.ts @@ -1,42 +1,61 @@ -import _ from 'lodash'; +import { throttle, uniq } from 'lodash'; import moment from 'moment'; -import { MessageModel } from '../models/message'; -import { messageExpired } from '../state/ducks/conversations'; +import { messagesExpired } from '../state/ducks/conversations'; import { TimerOptionsArray } from '../state/ducks/timerOptions'; import { LocalizerKeys } from '../types/LocalizerKeys'; import { initWallClockListener } from './wallClockListener'; import { Data } from '../data/data'; +import { getConversationController } from '../session/conversations'; + +export async function destroyMessagesAndUpdateRedux( + messages: Array<{ + conversationKey: string; + messageId: string; + }> +) { + if (!messages.length) { + return; + } + const conversationWithChanges = uniq(messages.map(m => m.conversationKey)); + + try { + // Delete all thoses messages in a single sql call + await Data.removeMessagesByIds(messages.map(m => m.messageId)); + } catch (e) { + window.log.error('destroyMessages: removeMessagesByIds failed', e && e.message ? e.message : e); + } + // trigger a redux update if needed for all those messages + window.inboxStore?.dispatch(messagesExpired(messages)); + + // trigger a refresh the last message for all those uniq conversation + conversationWithChanges.map(convoIdToUpdate => { + getConversationController() + .get(convoIdToUpdate) + ?.updateLastMessage(); + }); +} async function destroyExpiredMessages() { try { window.log.info('destroyExpiredMessages: Loading messages...'); const messages = await Data.getExpiredMessages(); - await Promise.all( - messages.map(async (message: MessageModel) => { - window.log.info('Message expired', { - sentAt: message.get('sent_at'), - }); - - // We delete after the trigger to allow the conversation time to process - // the expiration before the message is removed from the database. - await Data.removeMessage(message.id); - - // trigger the expiration of the message on the redux itself. - window.inboxStore?.dispatch( - messageExpired({ - conversationKey: message.attributes.conversationId, - messageId: message.id, - }) - ); - - const conversation = message.getConversation(); - if (conversation) { - await conversation.onExpired(message); - } - }) - ); + const messagesExpiredDetails: Array<{ + conversationKey: string; + messageId: string; + }> = messages.map(m => ({ + conversationKey: m.attributes.conversationId, + messageId: m.id, + })); + + messages.map(expired => { + window.log.info('Message expired', { + sentAt: expired.get('sent_at'), + }); + }); + + await destroyMessagesAndUpdateRedux(messagesExpiredDetails); } catch (error) { window.log.error( 'destroyExpiredMessages: Error deleting expired messages', @@ -81,7 +100,7 @@ async function checkExpiringMessages() { } timeout = global.setTimeout(destroyExpiredMessages, wait); } -const throttledCheckExpiringMessages = _.throttle(checkExpiringMessages, 1000); +const throttledCheckExpiringMessages = throttle(checkExpiringMessages, 1000); let isInit = false; From ad03fbd497349709e0b2380f30dbeb381e656ed4 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Fri, 7 Oct 2022 15:35:09 +1100 Subject: [PATCH 03/10] fix: skip recent deleted message empty react changes --- .../apis/open_group_api/sogsv3/sogsApiV3.ts | 58 ++++++++----------- .../sogsv3/sogsRollingDeletions.ts | 32 ++++++++++ ts/session/utils/RingBuffer.ts | 31 ++++++++++ 3 files changed, 88 insertions(+), 33 deletions(-) create mode 100644 ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts create mode 100644 ts/session/utils/RingBuffer.ts diff --git a/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts b/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts index 79c1880f3..b07b36f58 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts @@ -36,6 +36,7 @@ import { createSwarmMessageSentFromUs } from '../../../../models/messageFactory' import { Data } from '../../../../data/data'; import { processMessagesUsingCache } from './sogsV3MutationCache'; import { destroyMessagesAndUpdateRedux } from '../../../../util/expiringMessages'; +import { sogsRollingDeletions } from './sogsRollingDeletions'; /** * Get the convo matching those criteria and make sure it is an opengroup convo, or return null. @@ -155,29 +156,28 @@ async function filterOutMessagesInvalidSignature( return signaturesValidMessages; } -let totalDeletedMessages = 0; const handleSogsV3DeletedMessages = async ( messages: Array, serverUrl: string, roomId: string ) => { - const deletions = messages.filter(m => Boolean(m.deleted)); - const exceptDeletion = messages.filter(m => !m.deleted); - if (!deletions.length) { - return messages; + const messagesDeleted = messages.filter(m => Boolean(m.deleted)); + const messagesWithoutDeleted = messages.filter(m => !m.deleted); + if (!messagesDeleted.length) { + return messagesWithoutDeleted; } - totalDeletedMessages += deletions.length; - console.warn( - JSON.stringify({ - totalDeletedMessages, - }) - ); - const allIdsRemoved = deletions.map(m => m.id); + + const allIdsRemoved = messagesDeleted.map(m => m.id); + try { const convoId = getOpenGroupV2ConversationId(serverUrl, roomId); const convo = getConversationController().get(convoId); const messageIds = await Data.getMessageIdsFromServerIds(allIdsRemoved, convo.id); + allIdsRemoved.forEach(removedId => { + sogsRollingDeletions.addMessageDeletedId(convoId, removedId); + }); + if (messageIds && messageIds.length) { await destroyMessagesAndUpdateRedux( messageIds.map(messageId => ({ @@ -189,14 +189,9 @@ const handleSogsV3DeletedMessages = async ( } catch (e) { window?.log?.warn('handleDeletions failed:', e); } - return exceptDeletion; + return messagesWithoutDeleted; }; -// tslint:disable-next-line: one-variable-per-declaration -let totalEmptyReactions = 0, - totalMessagesWithResolvedBlindedIdsIfFound = 0, - totalMessageReactions = 0; - // tslint:disable-next-line: max-func-body-length cyclomatic-complexity const handleMessagesResponseV4 = async ( messages: Array, @@ -296,8 +291,6 @@ const handleMessagesResponseV4 = async ( const incomingMessageSeqNo = compact(messages.map(n => n.seqno)); const maxNewMessageSeqNo = Math.max(...incomingMessageSeqNo); - totalMessagesWithResolvedBlindedIdsIfFound += messagesWithResolvedBlindedIdsIfFound.length; - for (let index = 0; index < messagesWithResolvedBlindedIdsIfFound.length; index++) { const msgToHandle = messagesWithResolvedBlindedIdsIfFound[index]; try { @@ -323,25 +316,24 @@ const handleMessagesResponseV4 = async ( await OpenGroupData.saveV2OpenGroupRoom(roomInfosRefreshed); const messagesWithReactions = messages.filter(m => m.reactions !== undefined); - const messagesWithEmptyReactions = messagesWithReactions.filter(m => isEmpty(m.reactions)); - - totalMessageReactions += messagesWithReactions.length; - totalEmptyReactions += messagesWithEmptyReactions.length; - console.warn( - JSON.stringify({ - totalMessagesWithResolvedBlindedIdsIfFound, - totalMessageReactions, - totalEmptyReactions, - }) - ); if (messagesWithReactions.length > 0) { const conversationId = getOpenGroupV2ConversationId(serverUrl, roomId); const groupConvo = getConversationController().get(conversationId); if (groupConvo && groupConvo.isOpenGroupV2()) { - for (const message of messagesWithReactions) { + for (const messageWithReaction of messagesWithReactions) { + if (isEmpty(messageWithReaction.reactions)) { + /* + * When a message is deleted from the server, we get the deleted event as a data: null on the message itself + * and an update on its reactions. + * But, because we just deleted that message, we can skip trying to udpate its reactions: it's not in the DB anymore. + */ + if (sogsRollingDeletions.hasMessageDeletedId(conversationId, messageWithReaction.id)) { + continue; + } + } void groupConvo.queueJob(async () => { - await processMessagesUsingCache(serverUrl, roomId, message); + await processMessagesUsingCache(serverUrl, roomId, messageWithReaction); }); } } diff --git a/ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts b/ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts new file mode 100644 index 000000000..7b32360d8 --- /dev/null +++ b/ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts @@ -0,0 +1,32 @@ +import { RingBuffer } from '../../../utils/RingBuffer'; + +const rollingDeletedMessageIds: Map> = new Map(); + +// keep 2000 deleted message ids in memory +const perRoomRollingRemovedIds = 2000; + +const addMessageDeletedId = (conversationId: string, messageDeletedId: number) => { + if (!rollingDeletedMessageIds.has(conversationId)) { + rollingDeletedMessageIds.set(conversationId, new RingBuffer(perRoomRollingRemovedIds)); + } + const ringBuffer = rollingDeletedMessageIds.get(conversationId); + if (!ringBuffer) { + return; + } + ringBuffer.add(messageDeletedId); +}; + + +const hasMessageDeletedId = (conversationId: string, messageDeletedId: number) => { + if (!rollingDeletedMessageIds.has(conversationId)) { + return false; + } + + const messageIdWasDeletedRecently = rollingDeletedMessageIds + ?.get(conversationId) + ?.has(messageDeletedId); + + return messageIdWasDeletedRecently; +}; + +export const sogsRollingDeletions = { addMessageDeletedId, hasMessageDeletedId }; diff --git a/ts/session/utils/RingBuffer.ts b/ts/session/utils/RingBuffer.ts new file mode 100644 index 000000000..7a192e858 --- /dev/null +++ b/ts/session/utils/RingBuffer.ts @@ -0,0 +1,31 @@ +export class RingBuffer { + private buffer: Array = []; + private readonly capacity: number; + + constructor(capacity: number) { + this.capacity = capacity; + } + + public getCapacity(): number { + return this.capacity; + } + + public add(item: T) { + this.buffer.push(item); + this.crop(); + } + + public has(item: T) { + return this.buffer.includes(item); + } + + public clear() { + this.buffer = []; + } + + private crop() { + while (this.buffer.length > this.capacity) { + this.buffer.shift(); + } + } +} From c617976be03ad42d33f72d90b0e50a0457d51587 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Fri, 7 Oct 2022 16:19:58 +1100 Subject: [PATCH 04/10] test: added tests for RingBuffer & sogsRollingDeletions --- .../sogsv3/sogsRollingDeletions.ts | 28 ++++- .../open_group_api/sogsv3/sogsV3BatchPoll.ts | 3 +- ts/session/utils/RingBuffer.ts | 9 ++ .../unit/sogsv3/sogsRollingDeletions_test.ts | 71 ++++++++++++ ts/test/session/unit/utils/RingBuffer_test.ts | 106 ++++++++++++++++++ 5 files changed, 209 insertions(+), 8 deletions(-) create mode 100644 ts/test/session/unit/sogsv3/sogsRollingDeletions_test.ts create mode 100644 ts/test/session/unit/utils/RingBuffer_test.ts diff --git a/ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts b/ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts index 7b32360d8..80bbb8de2 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts @@ -2,12 +2,12 @@ import { RingBuffer } from '../../../utils/RingBuffer'; const rollingDeletedMessageIds: Map> = new Map(); -// keep 2000 deleted message ids in memory -const perRoomRollingRemovedIds = 2000; - const addMessageDeletedId = (conversationId: string, messageDeletedId: number) => { if (!rollingDeletedMessageIds.has(conversationId)) { - rollingDeletedMessageIds.set(conversationId, new RingBuffer(perRoomRollingRemovedIds)); + rollingDeletedMessageIds.set( + conversationId, + new RingBuffer(sogsRollingDeletions.getPerRoomCount()) + ); } const ringBuffer = rollingDeletedMessageIds.get(conversationId); if (!ringBuffer) { @@ -16,7 +16,6 @@ const addMessageDeletedId = (conversationId: string, messageDeletedId: number) = ringBuffer.add(messageDeletedId); }; - const hasMessageDeletedId = (conversationId: string, messageDeletedId: number) => { if (!rollingDeletedMessageIds.has(conversationId)) { return false; @@ -29,4 +28,21 @@ const hasMessageDeletedId = (conversationId: string, messageDeletedId: number) = return messageIdWasDeletedRecently; }; -export const sogsRollingDeletions = { addMessageDeletedId, hasMessageDeletedId }; +/** + * emptyMessageDeleteIds should only be used for testing purposes. + */ +const emptyMessageDeleteIds = () => { + rollingDeletedMessageIds.clear(); +}; + +export const sogsRollingDeletions = { + addMessageDeletedId, + hasMessageDeletedId, + emptyMessageDeleteIds, + getPerRoomCount, +}; + +// keep 2000 deleted message ids in memory +function getPerRoomCount() { + return 2000; +} diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3BatchPoll.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3BatchPoll.ts index cef51e209..7461b16f4 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3BatchPoll.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3BatchPoll.ts @@ -241,8 +241,7 @@ const makeBatchRequestPayload = ( method: 'GET', path: isNumber(options.messages.sinceSeqNo) ? `/room/${options.messages.roomId}/messages/since/${options.messages.sinceSeqNo}?t=r&reactors=${Reactions.SOGSReactorsFetchCount}` - : // : `/room/${options.messages.roomId}/messages/since/180000?t=r&reactors=${Reactions.SOGSReactorsFetchCount}`, - `/room/${options.messages.roomId}/messages/recent?reactors=${Reactions.SOGSReactorsFetchCount}`, + : `/room/${options.messages.roomId}/messages/recent?reactors=${Reactions.SOGSReactorsFetchCount}`, }; } break; diff --git a/ts/session/utils/RingBuffer.ts b/ts/session/utils/RingBuffer.ts index 7a192e858..6d9173d6f 100644 --- a/ts/session/utils/RingBuffer.ts +++ b/ts/session/utils/RingBuffer.ts @@ -1,3 +1,8 @@ +/** + * This ringbuffer class can be used to keep a list of at most a size and removing old items first when the size is exceeded. + * Internally, it uses an array to keep track of the order, so two times the same item can exist in it. + * + */ export class RingBuffer { private buffer: Array = []; private readonly capacity: number; @@ -10,6 +15,10 @@ export class RingBuffer { return this.capacity; } + public getLength(): number { + return this.buffer.length; + } + public add(item: T) { this.buffer.push(item); this.crop(); diff --git a/ts/test/session/unit/sogsv3/sogsRollingDeletions_test.ts b/ts/test/session/unit/sogsv3/sogsRollingDeletions_test.ts new file mode 100644 index 000000000..922b85f41 --- /dev/null +++ b/ts/test/session/unit/sogsv3/sogsRollingDeletions_test.ts @@ -0,0 +1,71 @@ +import { expect } from 'chai'; +import Sinon from 'sinon'; +import { sogsRollingDeletions } from '../../../../session/apis/open_group_api/sogsv3/sogsRollingDeletions'; + +describe('sogsRollingDeletions', () => { + beforeEach(() => { + sogsRollingDeletions.emptyMessageDeleteIds(); + Sinon.stub(sogsRollingDeletions, 'getPerRoomCount').returns(5); + }); + + afterEach(() => { + Sinon.restore(); + }); + + it('no items at all returns false', () => { + expect(sogsRollingDeletions.hasMessageDeletedId('convo1', 1)).to.be.equal( + false, + '1 should not be there' + ); + }); + + it('no items in that convo returns false', () => { + sogsRollingDeletions.addMessageDeletedId('convo1', 1); + + expect(sogsRollingDeletions.hasMessageDeletedId('convo2', 1)).to.be.equal( + false, + '1 should not be there' + ); + }); + + it('can add 1 item', () => { + sogsRollingDeletions.addMessageDeletedId('convo1', 1); + expect(sogsRollingDeletions.hasMessageDeletedId('convo1', 1)).to.be.equal( + true, + '1 should be there' + ); + }); + + it('can add more than capacity items', () => { + sogsRollingDeletions.addMessageDeletedId('convo1', 1); + sogsRollingDeletions.addMessageDeletedId('convo1', 2); + sogsRollingDeletions.addMessageDeletedId('convo1', 3); + sogsRollingDeletions.addMessageDeletedId('convo1', 4); + sogsRollingDeletions.addMessageDeletedId('convo1', 5); + sogsRollingDeletions.addMessageDeletedId('convo1', 6); + expect(sogsRollingDeletions.hasMessageDeletedId('convo1', 1)).to.be.equal( + false, + '1 should not be there' + ); + expect(sogsRollingDeletions.hasMessageDeletedId('convo1', 2)).to.be.equal( + true, + '2 should be there' + ); + expect(sogsRollingDeletions.hasMessageDeletedId('convo1', 3)).to.be.equal( + true, + '3 should be there' + ); + expect(sogsRollingDeletions.hasMessageDeletedId('convo1', 4)).to.be.equal( + true, + '4 should be there' + ); + expect(sogsRollingDeletions.hasMessageDeletedId('convo1', 5)).to.be.equal( + true, + '5 should be there' + ); + expect(sogsRollingDeletions.hasMessageDeletedId('convo1', 6)).to.be.equal( + true, + '6 should be there' + ); + }); +}); diff --git a/ts/test/session/unit/utils/RingBuffer_test.ts b/ts/test/session/unit/utils/RingBuffer_test.ts new file mode 100644 index 000000000..eee05f34c --- /dev/null +++ b/ts/test/session/unit/utils/RingBuffer_test.ts @@ -0,0 +1,106 @@ +// tslint:disable: no-implicit-dependencies max-func-body-length no-unused-expression no-require-imports no-var-requires + +import chai from 'chai'; +import { RingBuffer } from '../../../../session/utils/RingBuffer'; + +const { expect } = chai; + +describe('RingBuffer Utils', () => { + it('gets created with right capacity', () => { + const ring = new RingBuffer(5000); + expect(ring.getCapacity()).to.equal(5000); + expect(ring.getLength()).to.equal(0); + expect(ring.has(0)).to.equal(false, '4 should not be there'); + }); + + describe('length & capacity are right', () => { + it('length is right 1', () => { + const ring = new RingBuffer(4); + ring.add(0); + expect(ring.getLength()).to.equal(1); + }); + + it('length is right 4', () => { + const ring = new RingBuffer(4); + ring.add(0); + ring.add(1); + ring.add(2); + ring.add(3); + expect(ring.getLength()).to.equal(4); + }); + + it('capacity does not get exceeded', () => { + const ring = new RingBuffer(4); + ring.add(0); + ring.add(1); + ring.add(2); + ring.add(3); + ring.add(4); + expect(ring.getLength()).to.equal(4); + }); + }); + + it('items are removed in order 1', () => { + const ring = new RingBuffer(4); + ring.add(0); + ring.add(1); + ring.add(2); + ring.add(3); + ring.add(4); + expect(ring.has(0)).to.equal(false, '0 should not be there anymore'); + expect(ring.has(1)).to.equal(true, '1 should still be there'); + expect(ring.has(2)).to.equal(true, '2 should still be there'); + expect(ring.has(3)).to.equal(true, '3 should still be there'); + expect(ring.has(4)).to.equal(true, '4 should still be there'); + }); + + it('two times the same items can exist', () => { + const ring = new RingBuffer(4); + ring.add(0); + ring.add(1); + ring.add(2); + ring.add(1); + ring.add(4); + expect(ring.has(0)).to.equal(false, '0 should not be there anymore'); + expect(ring.has(1)).to.equal(true, '1 should still be there'); + expect(ring.has(2)).to.equal(true, '2 should still be there'); + expect(ring.has(3)).to.equal(false, '3 should not be there'); + expect(ring.has(4)).to.equal(true, '4 should still be there'); + }); + + it('items are removed in order completely', () => { + const ring = new RingBuffer(4); + ring.add(0); + ring.add(1); + ring.add(2); + ring.add(3); + ring.add(10); + ring.add(20); + ring.add(30); + ring.add(40); + expect(ring.has(0)).to.equal(false, '0 should not be there anymore'); + expect(ring.has(1)).to.equal(false, '1 should not be there'); + expect(ring.has(2)).to.equal(false, '2 should not be there'); + expect(ring.has(3)).to.equal(false, '3 should not be there'); + expect(ring.has(4)).to.equal(false, '4 should not be there'); + + expect(ring.has(10)).to.equal(true, '10 should still be there'); + expect(ring.has(20)).to.equal(true, '20 should still be there'); + expect(ring.has(30)).to.equal(true, '30 should still be there'); + expect(ring.has(40)).to.equal(true, '40 should still be there'); + }); + + it('clear empties the list but keeps the capacity', () => { + const ring = new RingBuffer(4); + ring.add(0); + ring.add(1); + ring.add(2); + ring.add(1); + expect(ring.getLength()).to.equal(4); + expect(ring.getCapacity()).to.equal(4); + ring.clear(); + expect(ring.getCapacity()).to.equal(4); + + expect(ring.getLength()).to.equal(0); + }); +}); From 24af2dabfba1cb35413caaedcd923f8e991cec96 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Fri, 7 Oct 2022 16:28:54 +1100 Subject: [PATCH 05/10] fix: remove usused onReadMessage method --- ts/models/conversation.ts | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/ts/models/conversation.ts b/ts/models/conversation.ts index 3795f5720..19d816f1b 100644 --- a/ts/models/conversation.ts +++ b/ts/models/conversation.ts @@ -471,26 +471,6 @@ export class ConversationModel extends Backbone.Model { return true; } - public async onReadMessage(message: MessageModel, readAt: number) { - // 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 - // easily end up with messages never marked as read (our previous early read - // sync handling, read syncs never sent because app was offline) - - // We queue it because we often get a whole lot of read syncs at once, and - // their markRead calls could very easily overlap given the async pull from DB. - - // Lastly, we don't send read syncs for any message marked read due to a read - // sync. That's a notification explosion we don't need. - return this.queueJob(() => - this.markReadBouncy(message.get('received_at') as any, { - sendReadReceipts: false, - readAt, - }) - ); - } - public async getUnreadCount() { const unreadCount = await Data.getUnreadCountByConversation(this.id); From 1ce8fd597950be28134ab5d39b70942b33fe1943 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Mon, 10 Oct 2022 11:39:15 +1100 Subject: [PATCH 06/10] fix: make circular buffer not recreate an array on each overflow --- ts/data/data.ts | 2 +- .../apis/open_group_api/sogsv3/sogsApiV3.ts | 2 +- .../sogsv3/sogsRollingDeletions.ts | 2 +- ts/session/utils/RingBuffer.ts | 52 ++++- ts/test/session/unit/utils/RingBuffer_test.ts | 184 ++++++++++++++---- 5 files changed, 198 insertions(+), 44 deletions(-) diff --git a/ts/data/data.ts b/ts/data/data.ts index f7adbe688..a9b47e674 100644 --- a/ts/data/data.ts +++ b/ts/data/data.ts @@ -392,7 +392,7 @@ async function removeMessage(id: string): Promise { /** * Note: this method will not clean up external files, just delete from SQL. - * File are cleaned up on app start if they are not linked to any messages + * Files are cleaned up on app start if they are not linked to any messages * */ async function removeMessagesByIds(ids: Array): Promise { diff --git a/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts b/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts index b07b36f58..731fb6f34 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts @@ -326,7 +326,7 @@ const handleMessagesResponseV4 = async ( /* * When a message is deleted from the server, we get the deleted event as a data: null on the message itself * and an update on its reactions. - * But, because we just deleted that message, we can skip trying to udpate its reactions: it's not in the DB anymore. + * But, because we just deleted that message, we can skip trying to update its reactions: it's not in the DB anymore. */ if (sogsRollingDeletions.hasMessageDeletedId(conversationId, messageWithReaction.id)) { continue; diff --git a/ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts b/ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts index 80bbb8de2..9582c6e1b 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsRollingDeletions.ts @@ -13,7 +13,7 @@ const addMessageDeletedId = (conversationId: string, messageDeletedId: number) = if (!ringBuffer) { return; } - ringBuffer.add(messageDeletedId); + ringBuffer.insert(messageDeletedId); }; const hasMessageDeletedId = (conversationId: string, messageDeletedId: number) => { diff --git a/ts/session/utils/RingBuffer.ts b/ts/session/utils/RingBuffer.ts index 6d9173d6f..259a3962a 100644 --- a/ts/session/utils/RingBuffer.ts +++ b/ts/session/utils/RingBuffer.ts @@ -4,6 +4,8 @@ * */ export class RingBuffer { + private newest = -1; + private oldest = 0; private buffer: Array = []; private readonly capacity: number; @@ -16,25 +18,59 @@ export class RingBuffer { } public getLength(): number { - return this.buffer.length; + if (this.isEmpty()) { + return 0; + } + + // When only one item was added, newest = 0 and oldest = 0. + // When more than one item was added, but less than capacity, newest = nbItemsAdded & oldest = 0. + // As soon as we overflow, oldest is incremented to oldest+1 and newest rolls back to 0, + // so this test fails here and we have to extract the length based on the two parts instead. + if (this.newest >= this.oldest) { + return this.newest + 1; + } + const firstPart = this.capacity - this.oldest; + const secondPart = this.newest + 1; + return firstPart + secondPart; } - public add(item: T) { - this.buffer.push(item); - this.crop(); + public insert(item: T) { + // see comments in `getLength()` + this.newest = (this.newest + 1) % this.capacity; + if (this.buffer.length >= this.capacity) { + this.oldest = (this.oldest + 1) % this.capacity; + } + this.buffer[this.newest] = item; } public has(item: T) { - return this.buffer.includes(item); + // no items at all + if (this.isEmpty()) { + return false; + } + return this.toArray().includes(item); + } + + public isEmpty() { + return this.newest === -1; } public clear() { this.buffer = []; + this.newest = -1; + this.oldest = 0; } - private crop() { - while (this.buffer.length > this.capacity) { - this.buffer.shift(); + public toArray(): Array { + if (this.isEmpty()) { + return []; + } + + if (this.newest >= this.oldest) { + return this.buffer.slice(0, this.newest + 1); } + const firstPart = this.buffer.slice(this.oldest, this.capacity); + const secondPart = this.buffer.slice(0, this.newest + 1); + return [...firstPart, ...secondPart]; } } diff --git a/ts/test/session/unit/utils/RingBuffer_test.ts b/ts/test/session/unit/utils/RingBuffer_test.ts index eee05f34c..add8aa712 100644 --- a/ts/test/session/unit/utils/RingBuffer_test.ts +++ b/ts/test/session/unit/utils/RingBuffer_test.ts @@ -10,43 +10,80 @@ describe('RingBuffer Utils', () => { const ring = new RingBuffer(5000); expect(ring.getCapacity()).to.equal(5000); expect(ring.getLength()).to.equal(0); - expect(ring.has(0)).to.equal(false, '4 should not be there'); + expect(ring.has(0)).to.equal(false, '0 should not be there'); }); describe('length & capacity are right', () => { + it('length is right 0', () => { + const ring = new RingBuffer(4); + expect(ring.getLength()).to.equal(0); + }); + it('length is right 1', () => { const ring = new RingBuffer(4); - ring.add(0); + ring.insert(0); expect(ring.getLength()).to.equal(1); }); it('length is right 4', () => { const ring = new RingBuffer(4); - ring.add(0); - ring.add(1); - ring.add(2); - ring.add(3); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); expect(ring.getLength()).to.equal(4); }); it('capacity does not get exceeded', () => { const ring = new RingBuffer(4); - ring.add(0); - ring.add(1); - ring.add(2); - ring.add(3); - ring.add(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + ring.insert(4); expect(ring.getLength()).to.equal(4); }); }); + describe('isEmpty is correct', () => { + it('no items', () => { + const ring = new RingBuffer(4); + expect(ring.isEmpty()).to.equal(true, 'no items isEmpty should be true'); + }); + + it('length is right 1', () => { + const ring = new RingBuffer(4); + ring.insert(0); + expect(ring.isEmpty()).to.equal(false, '1 item isEmpty should be false'); + }); + + it('length is right 4', () => { + const ring = new RingBuffer(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + expect(ring.isEmpty()).to.equal(false, '4 items isEmpty should be false'); + }); + + it('more than capacity', () => { + const ring = new RingBuffer(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + ring.insert(4); + expect(ring.isEmpty()).to.equal(false, '5 item isEmpty should be false'); + }); + }); + it('items are removed in order 1', () => { const ring = new RingBuffer(4); - ring.add(0); - ring.add(1); - ring.add(2); - ring.add(3); - ring.add(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + ring.insert(4); expect(ring.has(0)).to.equal(false, '0 should not be there anymore'); expect(ring.has(1)).to.equal(true, '1 should still be there'); expect(ring.has(2)).to.equal(true, '2 should still be there'); @@ -56,11 +93,11 @@ describe('RingBuffer Utils', () => { it('two times the same items can exist', () => { const ring = new RingBuffer(4); - ring.add(0); - ring.add(1); - ring.add(2); - ring.add(1); - ring.add(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(1); + ring.insert(4); expect(ring.has(0)).to.equal(false, '0 should not be there anymore'); expect(ring.has(1)).to.equal(true, '1 should still be there'); expect(ring.has(2)).to.equal(true, '2 should still be there'); @@ -70,14 +107,14 @@ describe('RingBuffer Utils', () => { it('items are removed in order completely', () => { const ring = new RingBuffer(4); - ring.add(0); - ring.add(1); - ring.add(2); - ring.add(3); - ring.add(10); - ring.add(20); - ring.add(30); - ring.add(40); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + ring.insert(10); + ring.insert(20); + ring.insert(30); + ring.insert(40); expect(ring.has(0)).to.equal(false, '0 should not be there anymore'); expect(ring.has(1)).to.equal(false, '1 should not be there'); expect(ring.has(2)).to.equal(false, '2 should not be there'); @@ -92,10 +129,10 @@ describe('RingBuffer Utils', () => { it('clear empties the list but keeps the capacity', () => { const ring = new RingBuffer(4); - ring.add(0); - ring.add(1); - ring.add(2); - ring.add(1); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(1); expect(ring.getLength()).to.equal(4); expect(ring.getCapacity()).to.equal(4); ring.clear(); @@ -103,4 +140,85 @@ describe('RingBuffer Utils', () => { expect(ring.getLength()).to.equal(0); }); + + describe('toArray', () => { + it('empty buffer', () => { + const ring = new RingBuffer(4); + expect(ring.toArray()).to.deep.eq([]); + }); + + it('with 1', () => { + const ring = new RingBuffer(4); + ring.insert(0); + + expect(ring.toArray()).to.deep.eq([0]); + }); + + it('with 4', () => { + const ring = new RingBuffer(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + + expect(ring.toArray()).to.deep.eq([0, 1, 2, 3]); + }); + + it('with 5', () => { + const ring = new RingBuffer(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + ring.insert(4); + + expect(ring.toArray()).to.deep.eq([1, 2, 3, 4]); + }); + + it('more than 2 full laps erasing data', () => { + const ring = new RingBuffer(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + ring.insert(4); // first lap first item + ring.insert(5); + ring.insert(6); // first item in toArray should be this one + ring.insert(7); + ring.insert(8); // second lap first item + ring.insert(9); + + expect(ring.toArray()).to.deep.eq([6, 7, 8, 9]); + }); + }); + + describe('clear', () => { + it('empty buffer', () => { + const ring = new RingBuffer(4); + ring.clear(); + expect(ring.getCapacity()).to.deep.eq(4); + expect(ring.getLength()).to.deep.eq(0); + }); + + it('with 1', () => { + const ring = new RingBuffer(4); + ring.insert(0); + ring.clear(); + expect(ring.getCapacity()).to.deep.eq(4); + expect(ring.getLength()).to.deep.eq(0); + }); + + it('with 5', () => { + const ring = new RingBuffer(4); + ring.insert(0); + ring.insert(1); + ring.insert(2); + ring.insert(3); + ring.insert(4); + + ring.clear(); + expect(ring.getCapacity()).to.deep.eq(4); + expect(ring.getLength()).to.deep.eq(0); + }); + }); }); From a18e3ccbb8009d363f98b95b8f0643787cdfbc4a Mon Sep 17 00:00:00 2001 From: William Grant Date: Fri, 28 Oct 2022 10:51:33 +1100 Subject: [PATCH 07/10] feat: made yarn install use frozen lockfile by default --- .yarnrc | 1 + 1 file changed, 1 insertion(+) create mode 100644 .yarnrc diff --git a/.yarnrc b/.yarnrc new file mode 100644 index 000000000..b162bd8bc --- /dev/null +++ b/.yarnrc @@ -0,0 +1 @@ +--install.frozen-lockfile true From 323245686403f62c3a3ea517d082c87bc000a3b5 Mon Sep 17 00:00:00 2001 From: William Grant Date: Fri, 28 Oct 2022 16:07:26 +1100 Subject: [PATCH 08/10] fix: make updatei18nKeysType.py work with python 3 --- tools/updateI18nKeysType.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/updateI18nKeysType.py b/tools/updateI18nKeysType.py index 11dfb29cf..585a07490 100755 --- a/tools/updateI18nKeysType.py +++ b/tools/updateI18nKeysType.py @@ -19,7 +19,7 @@ with open(EN_FILE,'r') as jsonFile: data = json.load(jsonFile) keys = data.keys() - stringToWrite += json.dumps(keys, sort_keys=True).replace(',', '\n |').replace('"', '\'')[1:-1] + stringToWrite += json.dumps(list(keys), sort_keys=True).replace(',', '\n |').replace('"', '\'')[1:-1] stringToWrite += ';\n' From 984dbf777d4a87a7d9f6d5f198a06eefa3f8820b Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Tue, 8 Nov 2022 16:40:35 +1100 Subject: [PATCH 09/10] fix: do not lowercase roomId before joining sogs room --- .../apis/open_group_api/opengroupV2/ApiUtil.ts | 4 ++-- .../open_group_api/opengroupV2/JoinOpenGroupV2.ts | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/ts/session/apis/open_group_api/opengroupV2/ApiUtil.ts b/ts/session/apis/open_group_api/opengroupV2/ApiUtil.ts index 27567233c..e1aedf677 100644 --- a/ts/session/apis/open_group_api/opengroupV2/ApiUtil.ts +++ b/ts/session/apis/open_group_api/opengroupV2/ApiUtil.ts @@ -1,4 +1,4 @@ -import _, { compact, flatten, isString } from 'lodash'; +import _, { clone, compact, flatten, isString } from 'lodash'; import { allowOnlyOneAtATime } from '../../../utils/Promise'; import { updateDefaultRooms, @@ -80,7 +80,7 @@ export function hasExistingOpenGroup(server: string, roomId: string) { return false; } - const serverNotLowerCased = server; + const serverNotLowerCased = clone(server); const serverLowerCase = serverNotLowerCased.toLowerCase(); let serverUrl: URL | undefined; diff --git a/ts/session/apis/open_group_api/opengroupV2/JoinOpenGroupV2.ts b/ts/session/apis/open_group_api/opengroupV2/JoinOpenGroupV2.ts index 503cb0f76..d3650f82e 100644 --- a/ts/session/apis/open_group_api/opengroupV2/JoinOpenGroupV2.ts +++ b/ts/session/apis/open_group_api/opengroupV2/JoinOpenGroupV2.ts @@ -22,14 +22,14 @@ import { getOpenGroupManager } from './OpenGroupManagerV2'; // 143.198.213.255:80/main?public_key=658d29b91892a2389505596b135e76a53db6e11d613a51dbd3d0816adffb231c export function parseOpenGroupV2(urlWithPubkey: string): OpenGroupV2Room | undefined { - const lowerCased = urlWithPubkey.trim().toLowerCase(); + const trimmed = urlWithPubkey.trim(); try { - if (!openGroupV2CompleteURLRegex.test(lowerCased)) { + if (!openGroupV2CompleteURLRegex.test(trimmed)) { throw new Error('regex fail'); } // prefix the URL if it does not have a prefix - const prefixedUrl = prefixify(lowerCased); + const prefixedUrl = prefixify(trimmed); // new URL fails if the protocol is not explicit const url = new URL(prefixedUrl); @@ -43,7 +43,7 @@ export function parseOpenGroupV2(urlWithPubkey: string): OpenGroupV2Room | undef }; return room; } catch (e) { - window?.log?.error('Invalid Opengroup v2 join URL:', lowerCased, e); + window?.log?.error('Invalid Opengroup v2 join URL:', trimmed, e); } return undefined; } @@ -62,8 +62,8 @@ async function joinOpenGroupV2(room: OpenGroupV2Room, fromConfigMessage: boolean return; } - const serverUrl = room.serverUrl.toLowerCase(); - const roomId = room.roomId.toLowerCase(); + const serverUrl = room.serverUrl; + const roomId = room.roomId; const publicKey = room.serverPublicKey.toLowerCase(); const prefixedServer = prefixify(serverUrl); From 16d14043b80163010ce271733bc3a842310c47d6 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Wed, 9 Nov 2022 17:07:14 +1100 Subject: [PATCH 10/10] fix: use token from first room info to build conversationId for sogs --- .../opengroupV2/JoinOpenGroupV2.ts | 24 ++++++----- .../opengroupV2/OpenGroupManagerV2.ts | 42 ++++++++++++------- .../opengroupV2/OpenGroupServerPoller.ts | 3 +- 3 files changed, 42 insertions(+), 27 deletions(-) diff --git a/ts/session/apis/open_group_api/opengroupV2/JoinOpenGroupV2.ts b/ts/session/apis/open_group_api/opengroupV2/JoinOpenGroupV2.ts index d3650f82e..c4d0ad646 100644 --- a/ts/session/apis/open_group_api/opengroupV2/JoinOpenGroupV2.ts +++ b/ts/session/apis/open_group_api/opengroupV2/JoinOpenGroupV2.ts @@ -1,5 +1,6 @@ import _ from 'lodash'; import { OpenGroupV2Room } from '../../../../data/opengroups'; +import { ConversationModel } from '../../../../models/conversation'; import { getConversationController } from '../../../conversations'; import { PromiseUtils, ToastUtils } from '../../../utils'; @@ -57,9 +58,12 @@ export function parseOpenGroupV2(urlWithPubkey: string): OpenGroupV2Room | undef * @param room The room id to join * @param publicKey The server publicKey. It comes from the joining link. (or is already here for the default open group server) */ -async function joinOpenGroupV2(room: OpenGroupV2Room, fromConfigMessage: boolean): Promise { +async function joinOpenGroupV2( + room: OpenGroupV2Room, + fromConfigMessage: boolean +): Promise { if (!room.serverUrl || !room.roomId || room.roomId.length < 2 || !room.serverPublicKey) { - return; + return undefined; } const serverUrl = room.serverUrl; @@ -97,6 +101,7 @@ async function joinOpenGroupV2(room: OpenGroupV2Room, fromConfigMessage: boolean if (!fromConfigMessage) { await forceSyncConfigurationNowIfNeeded(); } + return conversation; } catch (e) { window?.log?.error('Could not join open group v2', e.message); throw e; @@ -154,24 +159,23 @@ export async function joinOpenGroupV2WithUIEvents( uiCallback?.({ loadingState: 'started', conversationKey: conversationID }); - await joinOpenGroupV2(parsedRoom, fromConfigMessage); + const convoCreated = await joinOpenGroupV2(parsedRoom, fromConfigMessage); - const isConvoCreated = getConversationController().get(conversationID); - if (isConvoCreated) { + if (convoCreated) { if (showToasts) { ToastUtils.pushToastSuccess( 'connectToServerSuccess', window.i18n('connectToServerSuccess') ); } - uiCallback?.({ loadingState: 'finished', conversationKey: conversationID }); + uiCallback?.({ loadingState: 'finished', conversationKey: convoCreated?.id }); return true; - } else { - if (showToasts) { - ToastUtils.pushToastError('connectToServerFail', window.i18n('connectToServerFail')); - } } + if (showToasts) { + ToastUtils.pushToastError('connectToServerFail', window.i18n('connectToServerFail')); + } + uiCallback?.({ loadingState: 'failed', conversationKey: conversationID }); } catch (error) { window?.log?.warn('got error while joining open group:', error.message); diff --git a/ts/session/apis/open_group_api/opengroupV2/OpenGroupManagerV2.ts b/ts/session/apis/open_group_api/opengroupV2/OpenGroupManagerV2.ts index 07cea658a..e003a6b2b 100644 --- a/ts/session/apis/open_group_api/opengroupV2/OpenGroupManagerV2.ts +++ b/ts/session/apis/open_group_api/opengroupV2/OpenGroupManagerV2.ts @@ -6,7 +6,7 @@ import { getOpenGroupV2ConversationId } from '../utils/OpenGroupUtils'; import { OpenGroupRequestCommonType } from './ApiUtil'; import { OpenGroupServerPoller } from './OpenGroupServerPoller'; -import _ from 'lodash'; +import _, { clone, isEqual } from 'lodash'; import autoBind from 'auto-bind'; import { ConversationTypeEnum } from '../../../../models/conversationAttributes'; import { openGroupV2GetRoomInfoViaOnionV4 } from '../sogsv3/sogsV3RoomInfos'; @@ -153,7 +153,7 @@ export class OpenGroupManagerV2 { roomId: string, serverPublicKey: string ): Promise { - const conversationId = getOpenGroupV2ConversationId(serverUrl, roomId); + let conversationId = getOpenGroupV2ConversationId(serverUrl, roomId); if (getConversationController().get(conversationId)) { // Url incorrect or server not compatible @@ -163,39 +163,49 @@ export class OpenGroupManagerV2 { // here, the convo does not exist. Make sure the db is clean too await OpenGroupData.removeV2OpenGroupRoom(conversationId); - const room: OpenGroupV2Room = { - serverUrl, - roomId, - conversationId, - serverPublicKey, - }; - try { + const room: OpenGroupV2Room = { + serverUrl, + roomId, + conversationId, + serverPublicKey, + }; + const updatedRoom = clone(room); // save the pubkey to the db right now, the request for room Info // will need it and access it from the db await OpenGroupData.saveV2OpenGroupRoom(room); + const roomInfos = await openGroupV2GetRoomInfoViaOnionV4({ serverPubkey: serverPublicKey, serverUrl, roomId, }); - if (!roomInfos) { + + if (!roomInfos || !roomInfos.id) { throw new Error('Invalid open group roomInfo result'); } + updatedRoom.roomId = roomInfos.id; + conversationId = getOpenGroupV2ConversationId(serverUrl, roomInfos.id); + updatedRoom.conversationId = conversationId; + if (!isEqual(room, updatedRoom)) { + await OpenGroupData.removeV2OpenGroupRoom(conversationId); + await OpenGroupData.saveV2OpenGroupRoom(updatedRoom); + } + const conversation = await getConversationController().getOrCreateAndWait( conversationId, ConversationTypeEnum.GROUP ); - room.imageID = roomInfos.imageId || undefined; - room.roomName = roomInfos.name || undefined; - room.capabilities = roomInfos.capabilities; - await OpenGroupData.saveV2OpenGroupRoom(room); + updatedRoom.imageID = roomInfos.imageId || undefined; + updatedRoom.roomName = roomInfos.name || undefined; + updatedRoom.capabilities = roomInfos.capabilities; + await OpenGroupData.saveV2OpenGroupRoom(updatedRoom); // mark active so it's not in the contacts list but in the conversation list // mark isApproved as this is a public chat conversation.set({ active_at: Date.now(), - displayNameInProfile: room.roomName, + displayNameInProfile: updatedRoom.roomName, isApproved: true, didApproveMe: true, isTrustedForAttachmentDownload: true, // we always trust attachments when sent to an opengroup @@ -203,7 +213,7 @@ export class OpenGroupManagerV2 { await conversation.commit(); // start polling this room - this.addRoomToPolledRooms([room]); + this.addRoomToPolledRooms([updatedRoom]); return conversation; } catch (e) { diff --git a/ts/session/apis/open_group_api/opengroupV2/OpenGroupServerPoller.ts b/ts/session/apis/open_group_api/opengroupV2/OpenGroupServerPoller.ts index 0ffafa19e..19025eacd 100644 --- a/ts/session/apis/open_group_api/opengroupV2/OpenGroupServerPoller.ts +++ b/ts/session/apis/open_group_api/opengroupV2/OpenGroupServerPoller.ts @@ -143,9 +143,10 @@ export class OpenGroupServerPoller { window?.log?.info('this is not the correct ServerPoller'); return; } - if (this.roomIdsToPoll.has(room.roomId)) { + if (this.roomIdsToPoll.has(room.roomId) || this.roomIdsToPoll.has(room.roomId.toLowerCase())) { window?.log?.info(`Removing ${room.roomId} from polling for ${this.serverUrl}`); this.roomIdsToPoll.delete(room.roomId); + this.roomIdsToPoll.delete(room.roomId.toLowerCase()); } else { window?.log?.info( `Cannot remove polling of ${room.roomId} as it is not polled on ${this.serverUrl}`