From f7106069094df9905c68d3f94c661cabeab548bf Mon Sep 17 00:00:00 2001 From: Mikunj Date: Wed, 10 Jun 2020 10:47:54 +1000 Subject: [PATCH 1/5] Added p-retry --- package.json | 1 + yarn.lock | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/package.json b/package.json index 9d9b4e713..ecd6e3a25 100644 --- a/package.json +++ b/package.json @@ -90,6 +90,7 @@ "node-fetch": "2.3.0", "node-sass": "4.9.3", "os-locale": "2.1.0", + "p-retry": "^4.2.0", "pify": "3.0.0", "protobufjs": "6.8.6", "rc-slider": "^8.7.1", diff --git a/yarn.lock b/yarn.lock index ebb34c0e3..41b5cd5dc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -398,6 +398,11 @@ dependencies: redux "^3.6.0" +"@types/retry@^0.12.0": + version "0.12.0" + resolved "https://registry.yarnpkg.com/@types/retry/-/retry-0.12.0.tgz#2b35eccfcee7d38cd72ad99232fbd58bffb3c84d" + integrity sha512-wWKOClTTiizcZhXnPY4wikVAwmdYHp8q6DmC+EJUzAMsycb7HB32Kh9RN4+0gExjmPmZSAQjgURXIGATPegAvA== + "@types/rimraf@2.0.2": version "2.0.2" resolved "https://registry.yarnpkg.com/@types/rimraf/-/rimraf-2.0.2.tgz#7f0fc3cf0ff0ad2a99bb723ae1764f30acaf8b6e" @@ -7086,6 +7091,14 @@ p-map@^1.1.1: resolved "https://registry.yarnpkg.com/p-map/-/p-map-1.2.0.tgz#e4e94f311eabbc8633a1e79908165fca26241b6b" integrity sha512-r6zKACMNhjPJMTl8KcFH4li//gkrXWfbD6feV8l6doRHlzljFWGJ2AP6iKaCJXyZmAUMOPtvbW7EXkbWO/pLEA== +p-retry@^4.2.0: + version "4.2.0" + resolved "https://registry.yarnpkg.com/p-retry/-/p-retry-4.2.0.tgz#ea9066c6b44f23cab4cd42f6147cdbbc6604da5d" + integrity sha512-jPH38/MRh263KKcq0wBNOGFJbm+U6784RilTmHjB/HM9kH9V8WlCpVUcdOmip9cjXOh6MxZ5yk1z2SjDUJfWmA== + dependencies: + "@types/retry" "^0.12.0" + retry "^0.12.0" + p-try@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/p-try/-/p-try-1.0.0.tgz#cbc79cdbaf8fd4228e13f621f2b1a237c1b207b3" @@ -8823,6 +8836,11 @@ ret@~0.1.10: resolved "https://registry.yarnpkg.com/ret/-/ret-0.1.15.tgz#b8a4825d5bdb1fc3f6f53c2bc33f81388681c7bc" integrity sha512-TTlYpa+OL+vMMNG24xSlQGEJ3B/RzEfUlLct7b5G/ytav+wPrplCpVMFuwzXbkecJrb6IYo1iFb0S9v37754mg== +retry@^0.12.0: + version "0.12.0" + resolved "https://registry.yarnpkg.com/retry/-/retry-0.12.0.tgz#1b42a6266a21f07421d1b0b54b7dc167b01c013b" + integrity sha1-G0KmJmoh8HQh0bC1S33BZ7AcATs= + rgb2hex@^0.1.9: version "0.1.10" resolved "https://registry.yarnpkg.com/rgb2hex/-/rgb2hex-0.1.10.tgz#4fdd432665273e2d5900434940ceba0a04c8a8a8" From 090f0e2c38ac0fd69a126e8bcab2926b163f81b4 Mon Sep 17 00:00:00 2001 From: Mikunj Date: Wed, 10 Jun 2020 11:37:45 +1000 Subject: [PATCH 2/5] Add retrying and tests --- ts/session/sending/MessageSender.ts | 22 +- ts/test/session/sending/MessageSender_test.ts | 191 ++++++++++++------ 2 files changed, 141 insertions(+), 72 deletions(-) diff --git a/ts/session/sending/MessageSender.ts b/ts/session/sending/MessageSender.ts index 78d5763c3..58c070d72 100644 --- a/ts/session/sending/MessageSender.ts +++ b/ts/session/sending/MessageSender.ts @@ -6,6 +6,7 @@ import { SignalService } from '../../protobuf'; import { UserUtil } from '../../util'; import { MessageEncrypter } from '../crypto'; import { lokiMessageAPI, lokiPublicChatAPI } from '../../window'; +import pRetry from 'p-retry'; // ================ Regular ================ @@ -14,13 +15,10 @@ export function canSendToSnode(): boolean { return Boolean(lokiMessageAPI); } -export async function send({ - device, - plainTextBuffer, - encryption, - timestamp, - ttl, -}: RawMessage): Promise { +export async function send( + { device, plainTextBuffer, encryption, timestamp, ttl }: RawMessage, + retries: number = 3 +): Promise { if (!canSendToSnode()) { throw new Error('lokiMessageAPI is not initialized.'); } @@ -33,8 +31,14 @@ export async function send({ const envelope = await buildEnvelope(envelopeType, timestamp, cipherText); const data = wrapEnvelope(envelope); - // TODO: Somehow differentiate between Retryable and Regular erros - return lokiMessageAPI.sendMessage(device, data, timestamp, ttl); + // pRetry doesn't count the first call as a retry + return pRetry( + async () => lokiMessageAPI.sendMessage(device, data, timestamp, ttl), + { + retries: retries - 1, + factor: 1, + } + ); } async function buildEnvelope( diff --git a/ts/test/session/sending/MessageSender_test.ts b/ts/test/session/sending/MessageSender_test.ts index 67da38618..8a6525ec8 100644 --- a/ts/test/session/sending/MessageSender_test.ts +++ b/ts/test/session/sending/MessageSender_test.ts @@ -21,11 +21,25 @@ describe('MessageSender', () => { TestUtils.restoreStubs(); }); + describe('canSendToSnode', () => { + it('should return the correct value', () => { + const stub = TestUtils.stubWindow('lokiMessageAPI', undefined); + expect(MessageSender.canSendToSnode()).to.equal( + false, + 'We cannot send if lokiMessageAPI is not set' + ); + stub.set(sandbox.createStubInstance(LokiMessageAPI)); + expect(MessageSender.canSendToSnode()).to.equal( + true, + 'We can send if lokiMessageAPI is set' + ); + }); + }); + describe('send', () => { const ourNumber = 'ourNumber'; let lokiMessageAPIStub: sinon.SinonStubbedInstance; - let messageEncyrptReturnEnvelopeType = - SignalService.Envelope.Type.CIPHERTEXT; + let encryptStub: sinon.SinonStub<[string, Uint8Array, EncryptionType]>; beforeEach(() => { // We can do this because LokiMessageAPI has a module export in it @@ -34,76 +48,87 @@ describe('MessageSender', () => { }); TestUtils.stubWindow('lokiMessageAPI', lokiMessageAPIStub); + encryptStub = sandbox.stub(MessageEncrypter, 'encrypt').resolves({ + envelopeType: SignalService.Envelope.Type.CIPHERTEXT, + cipherText: crypto.randomBytes(10), + }); + sandbox.stub(UserUtil, 'getCurrentDevicePubKey').resolves(ourNumber); - sandbox - .stub(MessageEncrypter, 'encrypt') - .callsFake(async (_device, plainTextBuffer, _type) => ({ - envelopeType: messageEncyrptReturnEnvelopeType, - cipherText: plainTextBuffer, - })); }); - it('should pass the correct values to lokiMessageAPI', async () => { - const device = '0'; - const timestamp = Date.now(); - const ttl = 100; - - await MessageSender.send({ + describe('retry', () => { + const rawMessage = { identifier: '1', - device, + device: '0', plainTextBuffer: crypto.randomBytes(10), encryption: EncryptionType.Signal, - timestamp, - ttl, + timestamp: Date.now(), + ttl: 100, + }; + + it('should not retry if an error occurred during encryption', async () => { + encryptStub.throws(new Error('Failed to encrypt.')); + const promise = MessageSender.send(rawMessage); + await expect(promise).is.rejectedWith('Failed to encrypt.'); + expect(lokiMessageAPIStub.sendMessage.callCount).to.equal(0); }); - const args = lokiMessageAPIStub.sendMessage.getCall(0).args; - expect(args[0]).to.equal(device); - expect(args[2]).to.equal(timestamp); - expect(args[3]).to.equal(ttl); - }); + it('should only call lokiMessageAPI once if no errors occured', async () => { + await MessageSender.send(rawMessage); + expect(lokiMessageAPIStub.sendMessage.callCount).to.equal(1); + }); - it('should correctly build the envelope', async () => { - messageEncyrptReturnEnvelopeType = SignalService.Envelope.Type.CIPHERTEXT; + it('should only retry the specified amount of times before throwing', async () => { + lokiMessageAPIStub.sendMessage.throws(new Error('API error')); + const retries = 2; + const promise = MessageSender.send(rawMessage, retries); + await expect(promise).is.rejectedWith('API error'); + expect(lokiMessageAPIStub.sendMessage.callCount).to.equal(retries); + }); - // This test assumes the encryption stub returns the plainText passed into it. - const plainTextBuffer = crypto.randomBytes(10); - const timestamp = Date.now(); + it('should not throw error if successful send occurs within the retry limit', async () => { + lokiMessageAPIStub.sendMessage + .onFirstCall() + .throws(new Error('API error')); + await MessageSender.send(rawMessage, 3); + expect(lokiMessageAPIStub.sendMessage.callCount).to.equal(2); + }); + }); - await MessageSender.send({ - identifier: '1', - device: '0', - plainTextBuffer, - encryption: EncryptionType.Signal, - timestamp, - ttl: 1, + describe('logic', () => { + let messageEncyrptReturnEnvelopeType = + SignalService.Envelope.Type.CIPHERTEXT; + + beforeEach(() => { + encryptStub.callsFake(async (_device, plainTextBuffer, _type) => ({ + envelopeType: messageEncyrptReturnEnvelopeType, + cipherText: plainTextBuffer, + })); }); - const data = lokiMessageAPIStub.sendMessage.getCall(0).args[1]; - const webSocketMessage = SignalService.WebSocketMessage.decode(data); - expect(webSocketMessage.request?.body).to.not.equal( - undefined, - 'Request body should not be undefined' - ); - expect(webSocketMessage.request?.body).to.not.equal( - null, - 'Request body should not be null' - ); + it('should pass the correct values to lokiMessageAPI', async () => { + const device = '0'; + const timestamp = Date.now(); + const ttl = 100; - const envelope = SignalService.Envelope.decode( - webSocketMessage.request?.body as Uint8Array - ); - expect(envelope.type).to.equal(SignalService.Envelope.Type.CIPHERTEXT); - expect(envelope.source).to.equal(ourNumber); - expect(envelope.sourceDevice).to.equal(1); - expect(toNumber(envelope.timestamp)).to.equal(timestamp); - expect(envelope.content).to.deep.equal(plainTextBuffer); - }); + await MessageSender.send({ + identifier: '1', + device, + plainTextBuffer: crypto.randomBytes(10), + encryption: EncryptionType.Signal, + timestamp, + ttl, + }); + + const args = lokiMessageAPIStub.sendMessage.getCall(0).args; + expect(args[0]).to.equal(device); + expect(args[2]).to.equal(timestamp); + expect(args[3]).to.equal(ttl); + }); - describe('UNIDENTIFIED_SENDER', () => { - it('should set the envelope source to be empty', async () => { + it('should correctly build the envelope', async () => { messageEncyrptReturnEnvelopeType = - SignalService.Envelope.Type.UNIDENTIFIED_SENDER; + SignalService.Envelope.Type.CIPHERTEXT; // This test assumes the encryption stub returns the plainText passed into it. const plainTextBuffer = crypto.randomBytes(10); @@ -132,13 +157,53 @@ describe('MessageSender', () => { const envelope = SignalService.Envelope.decode( webSocketMessage.request?.body as Uint8Array ); - expect(envelope.type).to.equal( - SignalService.Envelope.Type.UNIDENTIFIED_SENDER - ); - expect(envelope.source).to.equal( - '', - 'envelope source should be empty in UNIDENTIFIED_SENDER' - ); + expect(envelope.type).to.equal(SignalService.Envelope.Type.CIPHERTEXT); + expect(envelope.source).to.equal(ourNumber); + expect(envelope.sourceDevice).to.equal(1); + expect(toNumber(envelope.timestamp)).to.equal(timestamp); + expect(envelope.content).to.deep.equal(plainTextBuffer); + }); + + describe('UNIDENTIFIED_SENDER', () => { + it('should set the envelope source to be empty', async () => { + messageEncyrptReturnEnvelopeType = + SignalService.Envelope.Type.UNIDENTIFIED_SENDER; + + // This test assumes the encryption stub returns the plainText passed into it. + const plainTextBuffer = crypto.randomBytes(10); + const timestamp = Date.now(); + + await MessageSender.send({ + identifier: '1', + device: '0', + plainTextBuffer, + encryption: EncryptionType.Signal, + timestamp, + ttl: 1, + }); + + const data = lokiMessageAPIStub.sendMessage.getCall(0).args[1]; + const webSocketMessage = SignalService.WebSocketMessage.decode(data); + expect(webSocketMessage.request?.body).to.not.equal( + undefined, + 'Request body should not be undefined' + ); + expect(webSocketMessage.request?.body).to.not.equal( + null, + 'Request body should not be null' + ); + + const envelope = SignalService.Envelope.decode( + webSocketMessage.request?.body as Uint8Array + ); + expect(envelope.type).to.equal( + SignalService.Envelope.Type.UNIDENTIFIED_SENDER + ); + expect(envelope.source).to.equal( + '', + 'envelope source should be empty in UNIDENTIFIED_SENDER' + ); + }); }); }); }); From 3b8b9f7306f65cbf8f320eba87cf1b13a4be54f3 Mon Sep 17 00:00:00 2001 From: Mikunj Date: Wed, 10 Jun 2020 13:43:00 +1000 Subject: [PATCH 3/5] Added comments --- ts/session/crypto/MessageEncrypter.ts | 4 ++++ ts/session/sending/MessageSender.ts | 22 ++++++++++++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/ts/session/crypto/MessageEncrypter.ts b/ts/session/crypto/MessageEncrypter.ts index 660c2448f..2919d754a 100644 --- a/ts/session/crypto/MessageEncrypter.ts +++ b/ts/session/crypto/MessageEncrypter.ts @@ -4,6 +4,10 @@ import { libloki, libsignal, Signal, textsecure } from '../../window'; import { UserUtil } from '../../util'; import { CipherTextObject } from '../../../libtextsecure/libsignal-protocol'; +/** + * Add padding to a message buffer + * @param messageBuffer The buffer to add padding to. + */ export function padPlainTextBuffer(messageBuffer: Uint8Array): Uint8Array { const plaintext = new Uint8Array( getPaddedMessageLength(messageBuffer.byteLength + 1) - 1 diff --git a/ts/session/sending/MessageSender.ts b/ts/session/sending/MessageSender.ts index 58c070d72..c6753a04c 100644 --- a/ts/session/sending/MessageSender.ts +++ b/ts/session/sending/MessageSender.ts @@ -10,11 +10,19 @@ import pRetry from 'p-retry'; // ================ Regular ================ +/** + * Check if we can send to service nodes. + */ export function canSendToSnode(): boolean { // Seems like lokiMessageAPI is not always guaranteed to be initialized return Boolean(lokiMessageAPI); } +/** + * Send a message via service nodes. + * @param message The message to send. + * @param retries The amount of times to retry sending. + */ export async function send( { device, plainTextBuffer, encryption, timestamp, ttl }: RawMessage, retries: number = 3 @@ -31,7 +39,9 @@ export async function send( const envelope = await buildEnvelope(envelopeType, timestamp, cipherText); const data = wrapEnvelope(envelope); - // pRetry doesn't count the first call as a retry + // pRetry counts retries after making the first call. + // So a retry couunt of 3 means you make a request then if it fails you make another request 3 times until it succeeds. + // This means a total of 4 requests are being sent, where as for us when we want 3 retries we only want 3 requests sent. return pRetry( async () => lokiMessageAPI.sendMessage(device, data, timestamp, ttl), { @@ -80,9 +90,18 @@ function wrapEnvelope(envelope: SignalService.Envelope): Uint8Array { // ================ Open Group ================ +/** + * Send a message to an open group. + * @param message The open group message. + */ export async function sendToOpenGroup( message: OpenGroupMessage ): Promise { + /* + Note: Retrying wasn't added to this but it can be added in the future if needed. + The only problem is that `channelAPI.sendMessage` returns true/false and doesn't throw any error so we can never be sure why sending failed. + This should be fixed and we shouldn't rely on returning true/false, rather return nothing (success) or throw an error (failure) + */ const { group, quote, attachments, preview, body } = message; const channelAPI = await lokiPublicChatAPI.findOrCreateChannel( group.server, @@ -91,7 +110,6 @@ export async function sendToOpenGroup( ); // Don't think returning true/false on `sendMessage` is a good way - // We should either: return nothing (success) or throw an error (failure) return channelAPI.sendMessage({ quote, attachments: attachments || [], From 88f87c0a70011d7f4e3b5d28f555622521847dfe Mon Sep 17 00:00:00 2001 From: Mikunj Varsani Date: Wed, 10 Jun 2020 15:32:50 +1000 Subject: [PATCH 4/5] Update ts/session/sending/MessageSender.ts Co-authored-by: Audric Ackermann --- ts/session/sending/MessageSender.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ts/session/sending/MessageSender.ts b/ts/session/sending/MessageSender.ts index c6753a04c..b11b86c8e 100644 --- a/ts/session/sending/MessageSender.ts +++ b/ts/session/sending/MessageSender.ts @@ -40,7 +40,7 @@ export async function send( const data = wrapEnvelope(envelope); // pRetry counts retries after making the first call. - // So a retry couunt of 3 means you make a request then if it fails you make another request 3 times until it succeeds. + // So a retry count of 3 means you make a request then if it fails you make another request 3 times until it succeeds. // This means a total of 4 requests are being sent, where as for us when we want 3 retries we only want 3 requests sent. return pRetry( async () => lokiMessageAPI.sendMessage(device, data, timestamp, ttl), From 1c84fa6c026a011c92a0e150f6c9e37d92002cd4 Mon Sep 17 00:00:00 2001 From: Mikunj Date: Wed, 10 Jun 2020 15:55:32 +1000 Subject: [PATCH 5/5] Renamed retries to attempts --- ts/session/sending/MessageSender.ts | 13 ++++++------- ts/test/session/sending/MessageSender_test.ts | 6 +++--- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/ts/session/sending/MessageSender.ts b/ts/session/sending/MessageSender.ts index b11b86c8e..e0439a85c 100644 --- a/ts/session/sending/MessageSender.ts +++ b/ts/session/sending/MessageSender.ts @@ -20,17 +20,19 @@ export function canSendToSnode(): boolean { /** * Send a message via service nodes. + * * @param message The message to send. - * @param retries The amount of times to retry sending. + * @param attempts The amount of times to attempt sending. Minimum value is 1. */ export async function send( - { device, plainTextBuffer, encryption, timestamp, ttl }: RawMessage, - retries: number = 3 + message: RawMessage, + attempts: number = 3 ): Promise { if (!canSendToSnode()) { throw new Error('lokiMessageAPI is not initialized.'); } + const { device, plainTextBuffer, encryption, timestamp, ttl } = message; const { envelopeType, cipherText } = await MessageEncrypter.encrypt( device, plainTextBuffer, @@ -39,13 +41,10 @@ export async function send( const envelope = await buildEnvelope(envelopeType, timestamp, cipherText); const data = wrapEnvelope(envelope); - // pRetry counts retries after making the first call. - // So a retry count of 3 means you make a request then if it fails you make another request 3 times until it succeeds. - // This means a total of 4 requests are being sent, where as for us when we want 3 retries we only want 3 requests sent. return pRetry( async () => lokiMessageAPI.sendMessage(device, data, timestamp, ttl), { - retries: retries - 1, + retries: Math.max(attempts - 1, 0), factor: 1, } ); diff --git a/ts/test/session/sending/MessageSender_test.ts b/ts/test/session/sending/MessageSender_test.ts index 8a6525ec8..c534aef9e 100644 --- a/ts/test/session/sending/MessageSender_test.ts +++ b/ts/test/session/sending/MessageSender_test.ts @@ -80,10 +80,10 @@ describe('MessageSender', () => { it('should only retry the specified amount of times before throwing', async () => { lokiMessageAPIStub.sendMessage.throws(new Error('API error')); - const retries = 2; - const promise = MessageSender.send(rawMessage, retries); + const attempts = 2; + const promise = MessageSender.send(rawMessage, attempts); await expect(promise).is.rejectedWith('API error'); - expect(lokiMessageAPIStub.sendMessage.callCount).to.equal(retries); + expect(lokiMessageAPIStub.sendMessage.callCount).to.equal(attempts); }); it('should not throw error if successful send occurs within the retry limit', async () => {