From ad224822740369795a42c66c235c3f5912cca588 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Fri, 7 Oct 2022 12:22:15 +1100 Subject: [PATCH 1/6] 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 2/6] 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 3/6] 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 4/6] 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 5/6] 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 6/6] 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); + }); + }); });