interledgerjs / ilp-plugin-btp

This has been moved to the monorepo https://github.com/interledgerjs/interledgerjs
8 stars 7 forks source link

Unecessary behavior in _closeIncomingSocket? #26

Closed dino-rodriguez closed 6 years ago

dino-rodriguez commented 6 years ago

When a new web socket connection is detected, a web socket BTP instance checks if there is an existing web socket connection (this._incomingWs) and closes it:

socket.once('message', async (binaryAuthMessage: WebSocket.Data) => {
          try {
            authPacket = BtpPacket.deserialize(binaryAuthMessage)
            this._log.trace('got auth packet. packet=%j', authPacket)
            this._validateAuthPacket(authPacket)
            if (this._incomingWs) {
              this._closeIncomingSocket(this._incomingWs, authPacket)
            }
            this._incomingWs = socket
            socket.send(BtpPacket.serializeResponse(authPacket.requestId, []))
          }

However, it passes the auth packet from the new web socket connection to _closeIncomingSocket, and gets the request ID from this auth packet:

 _closeIncomingSocket (socket: WebSocket, authPacket: BtpPacket) {
    socket.removeAllListeners()
    socket.once('message', async (data: WebSocket.Data) => {
      try {
        socket.send(BtpPacket.serializeError({
          code: 'F00',
          name: 'NotAcceptedError',
          data: 'This connection has been ended because the user has opened a new connection',
          triggeredAt: new Date().toISOString()
        }, authPacket.requestId, []))
      } catch (e) {
        this._log.error('error responding on closed socket', e)
      }
      socket.close()
    })
  }

Why is it sending an error response to the old stream with the new request ID, if the new socket is also getting a valid response? Shouldn't this function just remove all listeners and close the socket gracefully? Or send a normal response to the user on the old socket (not error F00)?