fix: handle stream has no metadata event #1132

summer-ji-eng · 2021-11-05T07:47:06Z

When a user call BigTable server-stream API readrows, the end event get stuck when the BigTable is empty. This can be reproduce on local Java emulator, not on production and Node.js emulator.

The root cause is that grpc handle the proxy metadata differently, no metadata event emit for Java emulator. (https://github.com/grpc/grpc-node/blob/master/packages/grpc-js/src/call-stream.ts#L527)
However, gax emit the response event once the grpc stream received the metadata, since the metadata has never been received in Java emulator case, stream seems swallow the end event.

Fix: grpc guaranteed status event, so we emit the response when received status event for no metadata event.

Sample code Java emulator

public class Main {
  public static void main(String[] args) throws Exception {
    Server server = ServerBuilder.forPort(1234)
        .addService(new BigtableImplBase() {
          @Override
          public void readRows(ReadRowsRequest request,
              StreamObserver<ReadRowsResponse> responseObserver) {
            System.out.println("readRows!, request:: " + request);
            responseObserver.onCompleted();
          }
        })
        .build();
    server.start();
    server.awaitTermination();
  }
}

Please reach out @summer-ji-eng if you need more detailed and internal information.

summer-ji-eng · 2021-11-05T07:54:05Z

src/streamingCalls/streaming.ts

+        stream.emit('response', {
+          code: 200,
+          details: '',
+          message: 'OK',
+          metadata: status.metadata,


@bcoe @alexander-fenster @JustinBeckwith Can I get the feedback on the object of response event.
What I am doing here is try to consistent with response object see L156-L160, but seems like the status received here has same structure, but different code. For example, status receive using local Java emulator

{ code: 0, details: '', metadata: Metadata { internalRepr: Map(2) { 'content-type' => [Array], 'trailer' => [Array] }, options: {} } }

In my opinion, we should return status as it is. Change status code to 200 cause confusion. But both status and response event emit same data. So I am not too sure from the user perspectively.

Any suggestion? Thank you .

I think the code here is the grpc code not http:
https://grpc.github.io/grpc/core/md_doc_statuscodes.html

Actually, nvm, I think I'm wrong:
https://screenshot.googleplex.com/GXJfqu8DKHq3DFF

I would recommend not including status.metadata here. It contains different information than what you would get from the metadata event. I would also recommend otherwise matching the response event emitted in the metadata handler, instead of trying to use the information from the status. I think it is not really correct to say that "both status and response event emit same data". They have similar structures, and the specific fields can match in some cases, but they're not really describing the same thing: response seems to describe whether the response started successfully, while status describes whether the response completed successfully.

And just to make sure it is clear, the code in the status event is a gRPC status code, and the code in the response event is an HTTP status code. The HTTP status code is sent in the gRPC protocol, but it is not exposed in the gRPC API.

It seems like remapping 0 -> 200 is reasonable, but should we support other grpc status codes and check the repsonse?

Would look to @murgatroid99's advice.

Try to do the same thing in code, with removing the metadata.

stream.emit('response', { code: 200, details: '', message: 'OK', });

src/streamingCalls/streaming.ts

alexander-fenster · 2021-11-05T17:26:11Z

src/streamingCalls/streaming.ts

@@ -134,6 +159,7 @@ export class StreamProxy extends duplexify implements GRPCCallResult {
        message: 'OK',


Should this be also put inside if (!responseHasSent)?

I think it doesn't matter. The stream event sequence is metadata first, then status is the last event to be emit.
We only care when the status received, we don't emit the response twice.
@murgatroid99 WDYT

alexander-fenster · 2021-11-05T17:30:22Z

test/unit/streaming.ts

@@ -257,6 +257,71 @@ describe('streaming', () => {
    }, 50);
  });

+  it('Not receive metadata event', done => {


Do we have a test coverage to check the responseHasSent logic? Probably add a counter to the test and check that the response event was emitted just once.

Add two test cases to cover the responseHasSent:

Stream received both metadata and status events.

response should contains metadata field.

Stream received only status events.

response should has no metadata field.

src/streamingCalls/streaming.ts

bcoe · 2021-11-09T15:10:35Z

src/streamingCalls/streaming.ts

+    // Emit the 'response' event if stream has no 'metadata' event.
+    // This avoids stream swallow the other events, such as 'end'.
+    stream.on('status', () => {
+      if (!this._responseHasSent) {


Do we set _this.responseHasSent as we start to write the response, or after the response has finished?

If we set it at the end, this could be a race condition. If we set it at the beginning, this seems like it will work well to me.

I think it doesn't matter. The stream event sequence is metadata first, then status is the last event to be emit.
We only care when the status received, we don't emit the response twice.

bcoe · 2021-11-09T15:15:03Z

test/unit/streaming.ts

+      });
+      s.push(null);
+      setImmediate(() => {
+        s.emit('metadata', responseMetadata);


I'm curious if metadata happened to be a huge buffer, or if this was happening on a slow connectoin, is there a chance status would emit before metadata finished reading?

According to @murgatroid99, the status event is always sent after the last message.
I assume the metadata to be emitted early before status. However, if the status sent earlier, then the response emit from status event.

If there is a metadata event, it is always emitted first. If there are any data events, they are emitted after metadata. The status event is always emitted, and it is always after the others.

bcoe · 2021-11-09T15:15:42Z

src/streamingCalls/streaming.ts

+    stream.on('status', () => {
+      if (!this._responseHasSent) {
+        stream.emit('response', {
+          code: 200,


status always indicates a 200?

Try to do the same thing in code

bcoe · 2021-11-09T15:18:58Z

src/streamingCalls/streaming.ts

+        stream.emit('response', {
+          code: 200,
+          details: '',
+          message: 'OK',
+          metadata: status.metadata,


It seems like remapping 0 -> 200 is reasonable, but should we support other grpc status codes and check the repsonse?

Would look to @murgatroid99's advice.

bcoe

Left a couple nits, and some questions.

Can you point me to which library is listening on the response event?

Co-authored-by: Benjamin E. Coe <[email protected]>

summer-ji-eng · 2021-11-09T21:59:30Z

Left a couple nits, and some questions.

Can you point me to which library is listening on the response event?

According to the comment, response event is try to fix google-cloud-node.

bcoe

LGTM, thank you for the fix 👍

summer-ji-eng requested a review from a team as a code owner November 5, 2021 07:47

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 5, 2021

summer-ji-eng commented Nov 5, 2021

View reviewed changes

summer-ji-eng requested review from alexander-fenster, bcoe and JustinBeckwith November 5, 2021 07:54

alexander-fenster reviewed Nov 5, 2021

View reviewed changes

src/streamingCalls/streaming.ts Outdated Show resolved Hide resolved

alexander-fenster reviewed Nov 5, 2021

View reviewed changes

summer-ji-eng added 2 commits November 8, 2021 18:02

fix: handle stream has no metadata event

ba9e7fb

make private variable and add unit test

fb713b5

summer-ji-eng force-pushed the fix_stream_metadata branch from cbebcc1 to fb713b5 Compare November 9, 2021 02:02

fix lint

e02e23b

bcoe reviewed Nov 9, 2021

View reviewed changes

bcoe approved these changes Nov 9, 2021

View reviewed changes

Update src/streamingCalls/streaming.ts

252d3c8

Co-authored-by: Benjamin E. Coe <[email protected]>

alexander-fenster approved these changes Nov 9, 2021

View reviewed changes

summer-ji-eng mentioned this pull request Nov 10, 2021

[DO NOT REVIEW]chore: test latest alpha google-gax googleapis/nodejs-pubsub#1427

Closed

bcoe approved these changes Nov 10, 2021

View reviewed changes

summer-ji-eng merged commit ad29bc2 into main Nov 10, 2021

summer-ji-eng deleted the fix_stream_metadata branch November 10, 2021 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle stream has no metadata event #1132

fix: handle stream has no metadata event #1132

summer-ji-eng commented Nov 5, 2021

summer-ji-eng Nov 5, 2021

igorbernstein2 Nov 5, 2021

igorbernstein2 Nov 5, 2021

murgatroid99 Nov 8, 2021

bcoe Nov 9, 2021

summer-ji-eng Nov 9, 2021

alexander-fenster Nov 5, 2021

summer-ji-eng Nov 9, 2021

murgatroid99 Nov 9, 2021

alexander-fenster Nov 5, 2021

summer-ji-eng Nov 9, 2021

bcoe Nov 9, 2021

summer-ji-eng Nov 9, 2021

bcoe Nov 9, 2021

summer-ji-eng Nov 9, 2021

murgatroid99 Nov 9, 2021

bcoe Nov 9, 2021

summer-ji-eng Nov 9, 2021

bcoe Nov 9, 2021

bcoe left a comment

summer-ji-eng commented Nov 9, 2021

bcoe left a comment

		@@ -134,6 +159,7 @@ export class StreamProxy extends duplexify implements GRPCCallResult {
		message: 'OK',

fix: handle stream has no metadata event #1132

fix: handle stream has no metadata event #1132

Conversation

summer-ji-eng commented Nov 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe left a comment

Choose a reason for hiding this comment

summer-ji-eng commented Nov 9, 2021

bcoe left a comment

Choose a reason for hiding this comment