Bigtable error silently ignored

[kolodny]

• OS: OSX 10.12.6
• Node.js version: 8.5.0 and 6.x
• npm version: 5.3.0
• google-cloud-node version: [email protected] [email protected] [email protected]

Steps to reproduce

const bigtable = require('@google-cloud/bigtable');

const bigtableClient = bigtable();
const instance = bigtableClient.instance(process.env.INSTANCE_ID);
const table = instance.table('doesnotexist');

Promise.resolve()
  .then(() => table.getRows())
  .then(([rows]) => console.log(`Read ${rows.length}`))
  .catch(error => console.error(`caught ${error}`))

I chased this bug down to https://github.com/GoogleCloudPlatform/google-cloud-node/blob/a64ad81517045196cf5a3f468ea15aad1e2c25da/packages/common-grpc/src/service.js#L376-L385

This "fake" response call causes streamResponseHandled in retry-request to be set to true on the fake response, that has the consequence of never firing the error callback. https://github.com/stephenplusplus/retry-request/blob/4181eec8187c3603d4e4e68db1ee6ac27725afa3/index.js#L113-L133

I tried reverting the code in #1444 to see if I could replicate the bug referenced at #1443 to see if I could find a different solution that would avoid this nasty regression, but I wasn't able to repro (I always got a response). I suspect that the code can be safely reverted. Reverting that bit of code did fix the bug of silently ignoring erorrs!

Thanks!

[stephenplusplus]

This issue was moved to googleapis/nodejs-bigtable#8

[kolodny]

I suspect the underlying bug lives in google-cloud-node and was just manifesting this way in Bigtable. Either way awesome response time

[stephenplusplus]

Woops, that's a good point. Well, we'll see what @callmehiphop thinks and maybe having two issues will make us fix it twice as fast :)

[callmehiphop]

@kolodny is correct, this is definitely a core issue.

@stephenplusplus I think this might actually be a retry-request issue. It seems that after a response event is received retry-request will swallow any errors produced by the stream moving forward. IMO the fix here would be to something like

requestStream.on('error', err => retryStream.emit('error', err));

Around https://github.com/stephenplusplus/retry-request/blob/master/index.js#L176

[stephenplusplus]

Check this out, though: https://github.com/stephenplusplus/retry-request/blob/4181eec8187c3603d4e4e68db1ee6ac27725afa3/index.js#L114

[stephenplusplus]

Ah, I see what you're saying. At the point you linked, we know the steam is good to go, so forwarding errors then is what we should do.

[callmehiphop]

I understand, my suggestion is if the response comes through first then you re-emit any request errors on the stream returned to the user. Regardless of whether or not this is a grpc implementation issue, I find it strange that retry-request would swallow an errors that occur mid-stream.

[stephenplusplus]

I released a new retry-request that will forward errors from the request stream.

@callmehiphop will that fix this issue, or is there more to do from common-grpc?

[callmehiphop]

I'm pretty sure that fix should resolve this issue!

[stephenplusplus]

@kolodny please remove your local node_modules, reinstall, and let us know how it goes!

[kolodny]

Confirmed, the issue now appears fixed. Thanks!