grpc: listen for metadata event instead of status #1444

callmehiphop · 2016-07-21T23:39:20Z

This fixes an issue where a grpc readable stream would hang indefinitely because we weren't draining it. Instead of listening for a status event, which occurs after results have been processed, we're now going to listen instead for a metadata event.

callmehiphop · 2016-07-21T23:40:27Z

@imathews would you mind testing this branch out to see if it resolves your issues?

test/common/grpc-service.js

+        };
+
+        fakeStream
+          .on('response', function(resp) {


coveralls · 2016-07-21T23:41:41Z

Coverage remained the same at 100.0% when pulling e56bdbf on callmehiphop:bigtable-stream-bug into 5689f96 on GoogleCloudPlatform:master.

lib/common/grpc-service.js

@@ -325,7 +325,8 @@ GrpcService.prototype.requestStream = function(protoOpts, reqOpts) {

    request: function() {
      return service[protoOpts.method](reqOpts, grpcOpts)
-        .on('status', function(status) {
+        .on('metadata', function() {
+          var status = this.received_status || { code: 0 };


coveralls · 2016-07-21T23:49:51Z

Coverage remained the same at 100.0% when pulling 4032b43 on callmehiphop:bigtable-stream-bug into 5689f96 on GoogleCloudPlatform:master.

lib/common/grpc-service.js

-        .on('status', function(status) {
-          var grcpStatus = GrpcService.decorateStatus_(status);
+        .on('metadata', function() {
+          var grcpStatus = GrpcService.decorateStatus_({ code: 0 });


coveralls · 2016-07-22T18:08:14Z

Coverage remained the same at 100.0% when pulling d70602e on callmehiphop:bigtable-stream-bug into 5689f96 on GoogleCloudPlatform:master.

stephenplusplus · 2016-07-22T18:09:43Z

Thanks :D

imathews · 2016-07-22T21:00:46Z

This is now working beautifully. Many many thanks @callmehiphop for the quick resolution!

grpc: listen for metadata event instead of status

e56bdbf

callmehiphop added the api: bigtable Issues related to the Bigtable API. label Jul 21, 2016

callmehiphop assigned stephenplusplus Jul 21, 2016

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 21, 2016

stephenplusplus reviewed Jul 21, 2016
View reviewed changes

test/common/grpc-service.js

};

fakeStream

.on('response', function(resp) {

This comment was marked as spam.

Sign in to view

stephenplusplus reviewed Jul 21, 2016
View reviewed changes

listening for error events in unit tests

4032b43

stephenplusplus reviewed Jul 22, 2016
View reviewed changes

setting default status to OK

d70602e

stephenplusplus merged commit 9cdcef5 into googleapis:master Jul 22, 2016

adding comment about stubbed status code

43a1c0d

kolodny mentioned this pull request Nov 5, 2017

Bigtable error silently ignored #2724

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grpc: listen for metadata event instead of status #1444

grpc: listen for metadata event instead of status #1444

callmehiphop commented Jul 21, 2016

callmehiphop commented Jul 21, 2016

This comment was marked as spam.

coveralls commented Jul 21, 2016 •

edited

Loading

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

coveralls commented Jul 21, 2016 •

edited

Loading

This comment was marked as spam.

This comment was marked as spam.

coveralls commented Jul 22, 2016 •

edited

Loading

stephenplusplus commented Jul 22, 2016

imathews commented Jul 22, 2016

grpc: listen for metadata event instead of status #1444

grpc: listen for metadata event instead of status #1444

Conversation

callmehiphop commented Jul 21, 2016

callmehiphop commented Jul 21, 2016

This comment was marked as spam.

coveralls commented Jul 21, 2016 • edited Loading

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

coveralls commented Jul 21, 2016 • edited Loading

This comment was marked as spam.

This comment was marked as spam.

coveralls commented Jul 22, 2016 • edited Loading

stephenplusplus commented Jul 22, 2016

imathews commented Jul 22, 2016

coveralls commented Jul 21, 2016 •

edited

Loading

coveralls commented Jul 21, 2016 •

edited

Loading

coveralls commented Jul 22, 2016 •

edited

Loading