puppet-service icon indicating copy to clipboard operation
puppet-service copied to clipboard

UnhandledRejection when calling Streamed call

Open windmemory opened this issue 5 years ago • 3 comments
trafficstars

Refer to this https://stackoverflow.com/questions/21771220/error-handling-with-node-js-streams

Seems like the error event in the stream will not be piped along, so we need to find out a way of handling such errors elegantly.

  public async messageFile (id: string): Promise<FileBox> {
    log.verbose('PuppetHostie', 'messageFile(%s)', id)

    const request = new MessageFileStreamRequest()
    request.setId(id)

    if (!this.grpcClient) {
      throw new Error('Can not get file from message since no grpc client.')
    }
    // When the next line throw an rejection, it won't be passed down the pipeline
    const stream = this.grpcClient.messageFileStream(request)
    const fileBoxChunkStream = unpackFileBoxChunk(stream)
    return chunkStreamToFileBox(fileBoxChunkStream)
  }

See a repro code: https://github.com/windmemory/repro-grpc-bug/tree/728bb90e3beb2b061645743ebcb92010b0112349

windmemory avatar Nov 03 '20 14:11 windmemory

Thank you very much for putting all those information and links together, they are very valuable!

Yes you are right, the error event in the stream will not be piped along.

It seems that the pipeline() is what we need?

stream.pipeline() leaves dangling event listeners on the streams after the callback has been invoked. In the case of reuse of streams after failure, this can cause event listener leaks and swallowed errors.

Please feel free to submit PR for demonstrating how to fix this. And I believe the file box will also be affected.

huan avatar Nov 04 '20 01:11 huan

After some investigation, I found that we are trying to find a way to pass the error along the stream to the destination, but actually this is not the best way to handle these errors, since after we passed it all along to the end, we will miss the context of where the error happened. Without these information, it might be harder for us to find the root cause of a problem in the future.

For example:

const fileBox = puppetHostie.messageFileBox()
// ... A lot of logic running here
await fileBox.toFile()

Handle / Throw error at where the error happens

The exception should be thrown at line 1, when we call the messageFileBox(), then we know that the messageFileBox() is where got broken.

Pass error along the pipe to the end

The exception should be thrown at line 3, when we call the toFile() on fileBox, if there are complex operations between the first and third lines, then troubleshooting the problem will be much harder. And the error is not thrown at where it should be thrown.

windmemory avatar Nov 04 '20 04:11 windmemory

I tried to throw error in the error listener, and catch outside the function, but seems like the error listener's running context is different, and the error can not be caught.

Sample code: https://github.com/windmemory/repro-grpc-bug/tree/254440970f5c46bd4453f12a71c548d5b251e0a8

So with this information, I think we can design a way of passing error in the first packet in the stream. If no errors, then we can continue the data transfer.

windmemory avatar Nov 04 '20 08:11 windmemory