From 406a395649cb6fac75baf2ba5493e025ce52841e Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Fri, 14 Oct 2022 18:48:10 +0530 Subject: [PATCH 01/14] Adding limit of 3 retries to JSON RPC requests --- src/createStreamMiddleware.ts | 12 ++++++-- src/index.test.ts | 56 +++++++++++++++++++++++++++++++---- 2 files changed, 59 insertions(+), 9 deletions(-) diff --git a/src/createStreamMiddleware.ts b/src/createStreamMiddleware.ts index 6af03b8..24e4762 100644 --- a/src/createStreamMiddleware.ts +++ b/src/createStreamMiddleware.ts @@ -14,6 +14,7 @@ interface IdMapValue { res: PendingJsonRpcResponse; next: JsonRpcEngineNextCallback; end: JsonRpcEngineEndCallback; + retryCount?: number; } interface IdMap { @@ -102,7 +103,8 @@ export default function createStreamMiddleware(options: Options = {}) { function processResponse(res: PendingJsonRpcResponse) { const context = idMap[res.id as unknown as string]; if (!context) { - throw new Error(`StreamMiddleware - Unknown response id "${res.id}"`); + console.warn(`StreamMiddleware - Unknown response id "${res.id}"`); + return; } delete idMap[res.id as unknown as string]; @@ -129,8 +131,12 @@ export default function createStreamMiddleware(options: Options = {}) { * Retry pending requests. */ function retryStuckRequests() { - Object.values(idMap).forEach(({ req }) => { - // TODO: limiting retries could be implemented here + Object.values(idMap).forEach(({ req, retryCount = 0 }) => { + // Check for retry count below ensure that a request is not retried more than 3 times + if (!req.id || retryCount >= 3) { + return; + } + idMap[req.id].retryCount = retryCount + 1; sendToStream(req); }); } diff --git a/src/index.test.ts b/src/index.test.ts index 7c425a1..6ebda2d 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -109,17 +109,21 @@ describe('middleware and engine to stream', () => { const RECONNECTED = 'CONNECTED'; describe('retry logic in middleware connected to a port', () => { - it('retries requests on reconnect message', async () => { + let engineA: JsonRpcEngine | undefined = undefined; + let engineB; + let messages: any[] = []; + let messageConsumer: any; + beforeEach(() => { // create guest - const engineA = new JsonRpcEngine(); + engineA = new JsonRpcEngine(); const jsonRpcConnection = createStreamMiddleware({ retryOnMessage: RECONNECTED, }); engineA.push(jsonRpcConnection.middleware); // create port - let messageConsumer = noop; - const messages: any[] = []; + messageConsumer = noop; + messages = []; const extensionPort = { onMessage: { addListener: (cb: any) => { @@ -143,15 +147,17 @@ describe('retry logic in middleware connected to a port', () => { clientSideStream .pipe(connectionStream as unknown as Duplex) .pipe(clientSideStream); + }); + it('retries requests on reconnect message', async () => { // request and expected result const req1 = { id: 1, jsonrpc, method: 'test' }; const req2 = { id: 2, jsonrpc, method: 'test' }; const res = { id: 1, jsonrpc, result: 'test' }; // Initially sent once - const responsePromise1 = engineA.handle(req1); - engineA.handle(req2); + const responsePromise1 = engineA?.handle(req1); + engineA?.handle(req2); await artificialDelay(); expect(messages).toHaveLength(2); @@ -179,4 +185,42 @@ describe('retry logic in middleware connected to a port', () => { expect(messages).toHaveLength(5); }); + + it('requests are not retried more than 3 times', async () => { + // request and expected result + const req = { id: 1, jsonrpc, method: 'test' }; + + // Initially sent once, message count at 1 + engineA?.handle(req); + await artificialDelay(); + expect(messages).toHaveLength(1); + + // Reconnected, gets sent again message count increased to 2 + messageConsumer({ + method: RECONNECTED, + }); + await artificialDelay(); + expect(messages).toHaveLength(2); + + // Reconnected, gets sent again message count increased to 3 + messageConsumer({ + method: RECONNECTED, + }); + await artificialDelay(); + expect(messages).toHaveLength(3); + + // Reconnected, gets sent again message count increased to 4 + messageConsumer({ + method: RECONNECTED, + }); + await artificialDelay(); + expect(messages).toHaveLength(4); + + // Reconnected, request is not sent again gets sent again message count stays at 4 + messageConsumer({ + method: RECONNECTED, + }); + await artificialDelay(); + expect(messages).toHaveLength(4); + }); }); From eeac0237db8238b715166ca028c101355223883e Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Fri, 21 Oct 2022 11:23:15 +0200 Subject: [PATCH 02/14] fix --- src/index.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/index.test.ts b/src/index.test.ts index 6ebda2d..be04ab1 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -109,8 +109,7 @@ describe('middleware and engine to stream', () => { const RECONNECTED = 'CONNECTED'; describe('retry logic in middleware connected to a port', () => { - let engineA: JsonRpcEngine | undefined = undefined; - let engineB; + let engineA: JsonRpcEngine | undefined; let messages: any[] = []; let messageConsumer: any; beforeEach(() => { From 1f7e1147f6996f78023f2958394bd21517048d08 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Tue, 25 Oct 2022 12:41:03 +0400 Subject: [PATCH 03/14] Update src/createStreamMiddleware.ts Co-authored-by: Zbyszek Tenerowicz --- src/createStreamMiddleware.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/createStreamMiddleware.ts b/src/createStreamMiddleware.ts index 24e4762..b28c1b2 100644 --- a/src/createStreamMiddleware.ts +++ b/src/createStreamMiddleware.ts @@ -132,6 +132,7 @@ export default function createStreamMiddleware(options: Options = {}) { */ function retryStuckRequests() { Object.values(idMap).forEach(({ req, retryCount = 0 }) => { + // Avoid retrying requests without an id - they cannot have matching responses so retry logic doesn't apply // Check for retry count below ensure that a request is not retried more than 3 times if (!req.id || retryCount >= 3) { return; From 6b2c2280bf96ff454a29b408d7db173bc797418b Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Tue, 25 Oct 2022 13:26:25 +0400 Subject: [PATCH 04/14] fix --- src/createStreamMiddleware.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/createStreamMiddleware.ts b/src/createStreamMiddleware.ts index 24e4762..a00653e 100644 --- a/src/createStreamMiddleware.ts +++ b/src/createStreamMiddleware.ts @@ -103,6 +103,10 @@ export default function createStreamMiddleware(options: Options = {}) { function processResponse(res: PendingJsonRpcResponse) { const context = idMap[res.id as unknown as string]; if (!context) { + // In case response is received and corresponsing request is not present in idMap + // we only show warning and not throw error anymore. + // Reason for change is that due to MV3 we have added action replay capability + // Thus it can happen that a request ends up being submitted upto 3 retries and there are multiple responses console.warn(`StreamMiddleware - Unknown response id "${res.id}"`); return; } @@ -134,7 +138,9 @@ export default function createStreamMiddleware(options: Options = {}) { Object.values(idMap).forEach(({ req, retryCount = 0 }) => { // Check for retry count below ensure that a request is not retried more than 3 times if (!req.id || retryCount >= 3) { - return; + throw new Error( + `StreamMiddleware - Retry limit exceeded for request id "${req.id}"`, + ); } idMap[req.id].retryCount = retryCount + 1; sendToStream(req); From de055422d449728fed071984b1897ebb7519fba7 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 26 Oct 2022 12:20:41 +0400 Subject: [PATCH 05/14] fix --- src/createStreamMiddleware.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/createStreamMiddleware.ts b/src/createStreamMiddleware.ts index 14db38a..fbc3b0e 100644 --- a/src/createStreamMiddleware.ts +++ b/src/createStreamMiddleware.ts @@ -103,12 +103,7 @@ export default function createStreamMiddleware(options: Options = {}) { function processResponse(res: PendingJsonRpcResponse) { const context = idMap[res.id as unknown as string]; if (!context) { - // In case response is received and corresponsing request is not present in idMap - // we only show warning and not throw error anymore. - // Reason for change is that due to MV3 we have added action replay capability - // Thus it can happen that a request ends up being submitted upto 3 retries and there are multiple responses - console.warn(`StreamMiddleware - Unknown response id "${res.id}"`); - return; + throw new Error(`StreamMiddleware - Unknown response id "${res.id}"`); } delete idMap[res.id as unknown as string]; From c66375b02ed59f962bcd410392d11f19829d3b9c Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 26 Oct 2022 12:23:03 +0400 Subject: [PATCH 06/14] fix --- src/createStreamMiddleware.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/createStreamMiddleware.ts b/src/createStreamMiddleware.ts index fbc3b0e..14db38a 100644 --- a/src/createStreamMiddleware.ts +++ b/src/createStreamMiddleware.ts @@ -103,7 +103,12 @@ export default function createStreamMiddleware(options: Options = {}) { function processResponse(res: PendingJsonRpcResponse) { const context = idMap[res.id as unknown as string]; if (!context) { - throw new Error(`StreamMiddleware - Unknown response id "${res.id}"`); + // In case response is received and corresponsing request is not present in idMap + // we only show warning and not throw error anymore. + // Reason for change is that due to MV3 we have added action replay capability + // Thus it can happen that a request ends up being submitted upto 3 retries and there are multiple responses + console.warn(`StreamMiddleware - Unknown response id "${res.id}"`); + return; } delete idMap[res.id as unknown as string]; From 74cebda88e5caf23ec84f3c2f9edfb8abb81e047 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 26 Oct 2022 20:55:12 +0400 Subject: [PATCH 07/14] fix --- src/createStreamMiddleware.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/createStreamMiddleware.ts b/src/createStreamMiddleware.ts index 14db38a..fbc3b0e 100644 --- a/src/createStreamMiddleware.ts +++ b/src/createStreamMiddleware.ts @@ -103,12 +103,7 @@ export default function createStreamMiddleware(options: Options = {}) { function processResponse(res: PendingJsonRpcResponse) { const context = idMap[res.id as unknown as string]; if (!context) { - // In case response is received and corresponsing request is not present in idMap - // we only show warning and not throw error anymore. - // Reason for change is that due to MV3 we have added action replay capability - // Thus it can happen that a request ends up being submitted upto 3 retries and there are multiple responses - console.warn(`StreamMiddleware - Unknown response id "${res.id}"`); - return; + throw new Error(`StreamMiddleware - Unknown response id "${res.id}"`); } delete idMap[res.id as unknown as string]; From c5a8d85d07d706852c3c729d3ff024fd4ed83cfa Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 26 Oct 2022 21:00:59 +0400 Subject: [PATCH 08/14] fix --- jest.config.js | 6 +++--- src/index.test.ts | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/jest.config.js b/jest.config.js index ebfb41b..2f6a570 100644 --- a/jest.config.js +++ b/jest.config.js @@ -21,10 +21,10 @@ module.exports = { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 78.94, + branches: 85.71, functions: 100, - lines: 96.27, - statements: 96.27, + lines: 97.44, + statements: 97.44, }, }, diff --git a/src/index.test.ts b/src/index.test.ts index be04ab1..6ba7e13 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -185,7 +185,7 @@ describe('retry logic in middleware connected to a port', () => { expect(messages).toHaveLength(5); }); - it('requests are not retried more than 3 times', async () => { + it('throw error when requests are retried more than 3 times', async () => { // request and expected result const req = { id: 1, jsonrpc, method: 'test' }; @@ -216,10 +216,10 @@ describe('retry logic in middleware connected to a port', () => { expect(messages).toHaveLength(4); // Reconnected, request is not sent again gets sent again message count stays at 4 - messageConsumer({ - method: RECONNECTED, - }); - await artificialDelay(); - expect(messages).toHaveLength(4); + expect(() => { + messageConsumer({ + method: RECONNECTED, + }); + }).toThrow(); }); }); From 34669367c046afc8b6c572b7ad7784d829735a22 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 26 Oct 2022 21:03:16 +0400 Subject: [PATCH 09/14] fix --- src/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/index.test.ts b/src/index.test.ts index 6ba7e13..390becb 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -220,6 +220,6 @@ describe('retry logic in middleware connected to a port', () => { messageConsumer({ method: RECONNECTED, }); - }).toThrow(); + }).toThrowError(); }); }); From 2f02c24b056fe0e84b24b6936f8b5bea9570959f Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 26 Oct 2022 21:10:18 +0400 Subject: [PATCH 10/14] fix --- src/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/index.test.ts b/src/index.test.ts index 390becb..6ba7e13 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -220,6 +220,6 @@ describe('retry logic in middleware connected to a port', () => { messageConsumer({ method: RECONNECTED, }); - }).toThrowError(); + }).toThrow(); }); }); From f3114796c7ef07cfe448c127f0fc60b01b7ef8da Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 26 Oct 2022 21:13:52 +0400 Subject: [PATCH 11/14] fix --- src/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/index.test.ts b/src/index.test.ts index 6ba7e13..f51f0f7 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -220,6 +220,6 @@ describe('retry logic in middleware connected to a port', () => { messageConsumer({ method: RECONNECTED, }); - }).toThrow(); + }).toThrow('StreamMiddleware - Retry limit exceeded for request id'); }); }); From 010a78dfd78c09ee52c43135f236a53f581e7b93 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Thu, 27 Oct 2022 19:34:42 +0400 Subject: [PATCH 12/14] fix --- src/createStreamMiddleware.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/createStreamMiddleware.ts b/src/createStreamMiddleware.ts index fbc3b0e..320373b 100644 --- a/src/createStreamMiddleware.ts +++ b/src/createStreamMiddleware.ts @@ -133,7 +133,10 @@ export default function createStreamMiddleware(options: Options = {}) { Object.values(idMap).forEach(({ req, retryCount = 0 }) => { // Avoid retrying requests without an id - they cannot have matching responses so retry logic doesn't apply // Check for retry count below ensure that a request is not retried more than 3 times - if (!req.id || retryCount >= 3) { + if (!req.id) { + return; + } + if (retryCount >= 3) { throw new Error( `StreamMiddleware - Retry limit exceeded for request id "${req.id}"`, ); From 0b03bce854deb14075cd4a7ca505c32667cef123 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Thu, 27 Oct 2022 22:33:25 +0400 Subject: [PATCH 13/14] adding test case --- jest.config.js | 6 +++--- src/index.test.ts | 19 ++++++++++++++++++- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/jest.config.js b/jest.config.js index 2f6a570..d673fa4 100644 --- a/jest.config.js +++ b/jest.config.js @@ -21,10 +21,10 @@ module.exports = { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 85.71, + branches: 86.36, functions: 100, - lines: 97.44, - statements: 97.44, + lines: 97.48, + statements: 97.48, }, }, diff --git a/src/index.test.ts b/src/index.test.ts index f51f0f7..be30187 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -215,11 +215,28 @@ describe('retry logic in middleware connected to a port', () => { await artificialDelay(); expect(messages).toHaveLength(4); - // Reconnected, request is not sent again gets sent again message count stays at 4 + // Reconnected, error is thrrown when trying to resend request more that 3 times expect(() => { messageConsumer({ method: RECONNECTED, }); }).toThrow('StreamMiddleware - Retry limit exceeded for request id'); }); + + it('does not retry if the request has no id', async () => { + // request and expected result + const req = { id: undefined, jsonrpc, method: 'test' }; + + // Initially sent once, message count at 1 + engineA?.handle(req); + await artificialDelay(); + expect(messages).toHaveLength(1); + + // Reconnected, but request is not re-submitted + messageConsumer({ + method: RECONNECTED, + }); + await artificialDelay(); + expect(messages).toHaveLength(1); + }); }); From 54b29b3dfb0843593cfc8c950f168f464e29669e Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Thu, 27 Oct 2022 22:36:41 +0400 Subject: [PATCH 14/14] adding test case --- jest.config.js | 6 +++--- src/createStreamMiddleware.ts | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/jest.config.js b/jest.config.js index d673fa4..630cccb 100644 --- a/jest.config.js +++ b/jest.config.js @@ -21,10 +21,10 @@ module.exports = { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 86.36, + branches: 86.95, functions: 100, - lines: 97.48, - statements: 97.48, + lines: 97.51, + statements: 97.51, }, }, diff --git a/src/createStreamMiddleware.ts b/src/createStreamMiddleware.ts index 320373b..4e33a42 100644 --- a/src/createStreamMiddleware.ts +++ b/src/createStreamMiddleware.ts @@ -136,11 +136,13 @@ export default function createStreamMiddleware(options: Options = {}) { if (!req.id) { return; } + if (retryCount >= 3) { throw new Error( `StreamMiddleware - Retry limit exceeded for request id "${req.id}"`, ); } + idMap[req.id].retryCount = retryCount + 1; sendToStream(req); });