JsCommunity / json-rpc-peer

JSON-RPC 2 transport-agnostic library
13 stars 3 forks source link

Un-catched Promise Rejection when we have a bad behavior server #52

Closed huan closed 6 years ago

huan commented 6 years ago

We should deal with the id not exist problem when the server returns a json rpc response with an id that the client does not know. currently, we will get a un-catched promise rejection.

julien-f commented 6 years ago

Can you provide me with a script that reproduce the issue please?

huan commented 6 years ago

Reproducable Code

import {
  Peer
} from './'

async function main () {
  const client = new Peer()

  client.on('data', () => {
    const response = {
      id      : 'no-exist-id',
      jsonrpc : '2.0',
      result  : 'result-data',
      type    : 'response'
    }
    client.write(JSON.stringify(response))
  })
  await client.request('test')
}

main()

Output

$ ts-node src/t.ts 
(node:46778) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'resolve' of undefined
    at Peer.<anonymous> (/Users/zixia/git/json-rpc-peer/src/index.ts:122:8)
    at step (/Users/zixia/git/json-rpc-peer/src/index.ts:42:23)
    at Object.next (/Users/zixia/git/json-rpc-peer/src/index.ts:23:53)
    at /Users/zixia/git/json-rpc-peer/src/index.ts:17:71
    at new Promise (<anonymous>)
    at __awaiter (/Users/zixia/git/json-rpc-peer/src/index.ts:13:12)
    at Peer.exec (/Users/zixia/git/json-rpc-peer/src/index.ts:102:16)
    at Peer.write (/Users/zixia/git/json-rpc-peer/src/index.ts:238:10)
    at Peer.<anonymous> (/Users/zixia/git/json-rpc-peer/src/t.ts:15:12)
    at emitTwo (events.js:126:13)
(node:46778) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)
(node:46778) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
julien-f commented 6 years ago

An error event is emitted in case of error in write(), note that it will probably crash your process so you need to handle it (like for every streams).

The issue was that, without proper error listeners, the error was re-thrown inside the promise handler which was triggering the warning.

FYI, smaller reproducing code:

new (require('./')).default().write(
  JSON.stringify({
    id: 'no-exist-id',
    jsonrpc: '2.0',
    result: 'result-data',
    type: 'response'
  })
)
huan commented 6 years ago

Got it, so we need to check if the Error is TypeError: Cannot read property 'resolve' of undefined in the error event listener.

Could we add more information to the Error, like which id/method/response caused this error?

For example: Cannot find 'id' for 'response' of 'method' with 'result'