[grpc-js] cancelled method does not cancel the stream

[mad-it]

Problem description

grpc-js client does not emit cancelled event properly when cancel() method is invoked on ClientReadableStream streams.

When cancelled is invoked by the grpc-js client, an error is thrown, but the stream is not actually cancelled. This results in the server not being able to shut down because of an open stream.

Reproduction steps

Reproduction https://github.com/mad-it/grpc-js-bug

Environment

• OS name, version and architecture: Ubuntu 18.04
• Node version: 12.x
• Package name and version : "@grpc/grpc-js@1.1.3

[murgatroid99]

Here's what's happening in that test: when a request ends with an error, it emits an error event. If that event is not handled, it throws an error, which might be turning into a promise rejection because the test function is async, but the important thing is that it interrupts the program flow right before the client sends the signal to the server to cancel the call. I can fix that, but for now you can avoid the problem by adding an error event handler. You should do that anyway, so that that error doesn't bubble up and crash your program.

In this specific test, you can write stream.on('error', () => {}) on line 42 of src/index.spec.ts to make the test pass. If you do that, you will see that the word cancelled is logged from src/index.ts twice.

[mad-it]

I can fix that, but for now you can avoid the problem by adding an error event handler. You should do that anyway, so that that error doesn't bubble up and crash your program.

As you can see for now my workaround is :

try {
 stream.cancel();
} catch {
 stream.cancel();
}

Since this issue arose as part of our tests its less of a problem. But super thanks for the explanation :)

[murgatroid99]

Oh, that's what that try/catch block is for. That doesn't actually work around the problem, it just obscures it. The error still breaks the execution flow and prevents the internal cleanup code from finishing correctly. Honestly, I probably wouldn't have spent an hour digging through my code trying to figure out what was going on if I had seen that the error was thrown there.

It is coincidental that the error is thrown from stream.cancel() and that will change when the fix in the linked PR is published. You should always handle the error event so that you can handle all kinds of request failures, not just cancellations.

In the specific case of your test code, if you add the error event handler as suggested, the tests will pass and the server will see the cancelled event as suggested, even if you remove that try/catch block and the second stream.cancel call.

[murgatroid99]

I have published the fix for this. The cancellation signal will now be sent to the server even if the error event is not handled on the client. As a consequence of this, as mentioned in my previous comment, the error will no longer be thrown from the cancel method on the client.