fix: disallow two pending requests with the same id (#1628)

* only increment nextId if id used

* throw error if promise already pending with id

* add test

* modify existing tests

* fix bug + tests

src/client/requestManager.ts

@@ -1,4 +1,9 @@
-import { ResponseFormatError, RippledError, TimeoutError } from '../errors'
+import {
+  ResponseFormatError,
+  RippledError,
+  TimeoutError,
+  XrplError,
+} from '../errors'
 import { Response } from '../models/methods'
 import { BaseRequest, ErrorResponse } from '../models/methods/baseMethod'
 
@@ -77,6 +82,7 @@ export default class RequestManager {
   public rejectAll(error: Error): void {
     this.promisesAwaitingResponse.forEach((_promise, id, _map) => {
       this.reject(id, error)
+      this.deletePromise(id)
     })
   }
 
@@ -88,13 +94,19 @@ export default class RequestManager {
    * @param request - Request to create.
    * @param timeout - Timeout length to catch hung responses.
    * @returns Request ID, new request form, and the promise for resolving the request.
+   * @throws XrplError if request with the same ID is already pending.
    */
   public createRequest<T extends BaseRequest>(
     request: T,
     timeout: number,
   ): [string | number, string, Promise<Response>] {
-    const newId = request.id ? request.id : this.nextId
-    this.nextId += 1
+    let newId: string | number
+    if (request.id == null) {
+      newId = this.nextId
+      this.nextId += 1
+    } else {
+      newId = request.id
+    }
     const newRequest = JSON.stringify({ ...request, id: newId })
     const timer = setTimeout(
       () => this.reject(newId, new TimeoutError()),
@@ -106,6 +118,9 @@ export default class RequestManager {
     if (timer.unref) {
       timer.unref()
     }
+    if (this.promisesAwaitingResponse.has(newId)) {
+      throw new XrplError(`Response with id '${newId}' is already pending`)
+    }
     const newPromise = new Promise<Response>(
       (resolve: (value: Response | PromiseLike<Response>) => void, reject) => {
         this.promisesAwaitingResponse.set(newId, { resolve, reject, timer })
@@ -125,8 +140,7 @@ export default class RequestManager {
   public handleResponse(response: Partial<Response | ErrorResponse>): void {
     if (
       response.id == null ||
-      !Number.isInteger(response.id) ||
-      response.id < 0
+      !(typeof response.id === 'string' || typeof response.id === 'number')
     ) {
       throw new ResponseFormatError('valid id not found in response', response)
     }
@@ -165,7 +179,6 @@ export default class RequestManager {
    * @param id - ID of the request.
    */
   private deletePromise(id: string | number): void {
-    // eslint-disable-next-line @typescript-eslint/no-dynamic-delete -- Needs to delete promise after request has been fulfilled.
-    delete this.promisesAwaitingResponse[id]
+    this.promisesAwaitingResponse.delete(id)
   }
 }

test/client/submitSignedTransaction.ts

@@ -62,7 +62,7 @@ describe('client.submitSignedTransaction', function () {
 
     this.mockRippled.addResponse('submit', rippled.submit.success)
 
-    assertRejects(
+    await assertRejects(
       this.client.submitSignedTransaction(signedTx),
       ValidationError,
       'Transaction must be signed',

test/connection.ts

@@ -17,7 +17,7 @@ import { Connection } from 'xrpl-local/client/connection'
 
 import rippled from './fixtures/rippled'
 import { setupClient, teardownClient } from './setupClient'
-import { ignoreWebSocketDisconnect } from './testUtils'
+import { assertRejects, ignoreWebSocketDisconnect } from './testUtils'
 
 // how long before each test case times out
 const TIMEOUT = 20000
@@ -514,13 +514,13 @@ describe('Connection', function () {
     this.client.on('error', (errorCode, errorMessage, message) => {
       assert.strictEqual(errorCode, 'badMessage')
       assert.strictEqual(errorMessage, 'valid id not found in response')
-      assert.strictEqual(message, '{"type":"response","id":"must be integer"}')
+      assert.strictEqual(message, '{"type":"response","id":{}}')
       done()
     })
     this.client.connection.onMessage(
       JSON.stringify({
         type: 'response',
-        id: 'must be integer',
+        id: {},
       }),
     )
   })
@@ -619,4 +619,20 @@ describe('Connection', function () {
       .then(() => new Error('Should not have succeeded'))
       .catch(done())
   })
+
+  it('should throw error if pending response with same ID', async function () {
+    const promise1 = this.client.connection.request({
+      id: 'test',
+      command: 'ping',
+    })
+    const promise2 = this.client.connection.request({
+      id: 'test',
+      command: 'ping',
+    })
+    await assertRejects(
+      Promise.all([promise1, promise2]),
+      XrplError,
+      "Response with id 'test' is already pending",
+    )
+  })
 })

test/mockRippled.ts

@@ -180,6 +180,16 @@ export default function createMockRippled(port: number): MockedWebSocketServer {
       )
     } else if (request.data.closeServer) {
       conn.close()
+    } else if (request.data.delayedResponseIn) {
+      setTimeout(() => {
+        conn.send(
+          createResponse(request, {
+            status: 'success',
+            type: 'response',
+            result: {},
+          }),
+        )
+      }, request.data.delayedResponseIn)
     }
   }
 

test/testUtils.ts

@@ -58,7 +58,7 @@ export function assertResultMatch(
  * @param message - Expected error message/substring of the error message.
  */
 export async function assertRejects(
-  promise: PromiseLike<Record<string, unknown>>,
+  promise: PromiseLike<any>,
   instanceOf: any,
   message?: string | RegExp,
 ): Promise<void> {
@@ -72,7 +72,7 @@ export async function assertRejects(
 
     assert(error instanceof instanceOf, error.message)
     if (typeof message === 'string') {
-      assert.strictEqual(error.message, message)
+      assert.strictEqual(error.message, message, 'Messages do not match')
     } else if (message instanceof RegExp) {
       assert(message.test(error.message))
     }