mode=grpcwebtext not always triggering "end" event on stream

[johanbrandhorst]

Hi,

The grpc-web-compatibility-test repo recently automated all its tests (thanks @public 🎉). However, this exposed a problem that wasn't obvious when running tests manually:

It seems that streaming methods in the generated grpcwebtext mode sometimes fail to return successfully. Please see johanbrandhorst/grpc-web-compatibility-test#15 (comment) and johanbrandhorst/grpc-web-compatibility-test#13 for more discussion. Notably, the following can be seen in the console during failures:

index.js:44 Uncaught TypeError: b.i is not a function
    at S.<anonymous> (index.js:44)
    at sb (index.js:29)
    at L (index.js:28)
    at Zb (index.js:37)
    at S../node_modules/grpc-web/index.js.S.R (index.js:36)
    at S../node_modules/grpc-web/index.js.S.K (index.js:36)

(maybe related to #566?).

[stanley-cheung]

@public @johanbrandhorst Thanks for the report. Will definitely look into this. You mentioned that this is flaky? In what ways is this flaky? Does it mean that if you run the tests long enough (i.e. streaming and sending lots and lots of messages), that it eventually failed?

[johanbrandhorst]

No, it seems to fail almost every time in the testing scenario, surprisingly. The streaming call times out. But every now and then (1 in 10 test runs) it passes. That's what I mean by flaky.

[public]

Done a bit of research on this today. It's rather hard to reverse engineer whats going on since there's no sourcemaps in the published npm package and I can't get the build to work on my machine.

I am pretty sure that index.js:44 Uncaught TypeError: b.i is not a function is related but not the root cause.

This is the test case that's failing

        const req = new ServerStreamingEchoRequest();
        req.setMessageCount(5);
        var interval = new Duration();
        interval.setSeconds(1);
        req.setMessageInterval(interval);
        req.setMessage("test");
        const srv = client.serverStreamingEcho(req);

        var recvCount = 0;

        srv.on("data", function(msg: ServerStreamingEchoResponse) {
            assert(msg.getMessage() == "test");
            recvCount += 1;
        });

        srv.on("status", function(status: grpcWeb.Status) {
            assert(status.code == grpcWeb.StatusCode.OK);
        });

        srv.on("end", function() {
            assert(recvCount == 5);
            done();
        });

The tests don't actually fail on the b.i error, they fail on a timeout because done() is never called from our end handler.

If we delete the data event handler we always get the b.i error. Because the XHR is sent as soon as client.serverStreamingEcho(req); is called we have to race to get our event handlers attached before the XHR triggers any events. Generally this doesn't seem to happen when running the tests in normal (non-headless) Chrome but makes me think that there's some sort of race condition in the event handling. Is it possible for the XHR to start firing events before we've attached the handlers?

If we move the done to inside status instead we don't get any failures. But it seems very suspicious that end isn't firing consistently in either Chrome or Chrome Headless.

@johanbrandhorst Maybe we should just change our tests to only check for the status event? It's not entirely clear what's the difference in semantics for status vs end in this case.

[johanbrandhorst]

Maybe we should just change our tests to only check for the status event?

@public This sounds fine for our tests, but obviously we'll leave this issue open to clarify the interactions between status and end events.

[stanley-cheung]

@public, thanks for the investigation. In that case, this sounds very similar to issues like #289 and #543 where we had observed that the end event were not being fired by the browser, potentially due to some kind of race conditions. In the past users have used the workaround of listening to the status event, but I do want to understand the issue with end, although we haven't figured that one out yet.

[johanbrandhorst]

Closing this as duplicate of #543

[tariqulislam]

The issue will be solve by just add the empty functions stream.on("end", function (){})