grpc-js: stream.push() after EOF

[alexander-fenster]

Problem description

When trying to replace grpc with @grpc/grpc-js in GAPIC client libraries, many libraries' system tests randomly fail on Node 8 with the following stack trace:

     Error: stream.push() after EOF
      at readableAddChunk (_stream_readable.js:240:30)
      at ClientDuplexStreamImpl.Readable.push (_stream_readable.js:208:10)
      at Http2CallStream.call.on (node_modules/@grpc/grpc-js/build/src/call.js:36:21)
      at addChunk (_stream_readable.js:263:12)
      at readableAddChunk (_stream_readable.js:250:11)
      at Http2CallStream.Readable.push (_stream_readable.js:208:10)
      at Http2CallStream.handleFilteredRead (node_modules/@grpc/grpc-js/build/src/call-stream.js:75:23)
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:189:7)

Reproduction steps

The problem is flaky. I was only able to reproduce it on Node 8.15.0 (never on Node 10) (which may be related to the fact that _stream_readable.js was refactored in Node 10). I see this problem when I run system tests for Google Cloud Pub/Sub package, so to repro that, you'll need to have a service account key (contact me and I'll give you one).

git clone [email protected]:googleapis/nodejs-pubsub.git
cd nodejs-pubsub
npm install
export GOOGLE_APPLICATION_CREDENTIALS=/path/to/service-account-key.json
export GCLOUD_PROJECT=your-gcloud-project-id
export GOOGLE_CLOUD_USE_GRPC_JS=1
npm run system-test

The mocha error you'll likely see will be Error: done() called multiple times but the actual exception (not printed) will be the one I pasted above.

Environment

• OS name, version and architecture: Debian GNU/Linux rodete 4.18.10-1rodete2-amd64 x86_64, but can be reproduced on regular node:8 Docker image (e.g. the one we use in Kokoro)
• Node version: 8.15.0 (NOT 10.x)
• Node installation method: unpack .tgz
• Package name and version: @grpc/grpc-js v0.3.5

Additional details

I'm pretty sure the error might be related to the fact that Pub/Sub client library destroys the stream and this code is executed in the test:

https://github.com/googleapis/nodejs-pubsub/blob/eee4f5de6bd83be687422c5ccef8781fa5ac7af0/src/subscriber.ts#L244-L256

but (a) this code never fails in native gRPC, and (b) throwing internal Node exception is not good.

[callmehiphop]

Has there ever been any thoughts around using the readable-stream module instead of the native stream module?

[murgatroid99]

I've never seen that before. I think we have to weigh the value of stream changes exposing bugs in our code vs the downside of stream changes potentially breaking otherwise non-buggy code. I don't know yet which category this issue is in, and I don't know which I would expect to be more common.

[callmehiphop]

I've been testing a change on PubSub and so far it looks promising. Our issue looks like a call to destroy() on a client duplex stream after it has been cancelled. I think maybe the behavior change we are seeing is that data continues to be pushed in even after we've cancelled the request. I'm just guessing, mostly because I'm totally ignorant to how native grpc streams work, but this might be related to how Node streams buffer data and grpc-js continues to listen for data even after cancel and destroy have been called.