Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that the end callback is called #657

Merged
merged 2 commits into from
Oct 18, 2019

Conversation

vbfox
Copy link
Contributor

@vbfox vbfox commented Oct 17, 2019

This should fix the case where a server stream was closed by some element on the network (Like envoy proxy with a timeout configured) and grpc-web didn't report any even (Neither 'error' or 'end') this fixes for us symptoms similar to #543 (We also see ERR_INCOMPLETE_CHUNKED_ENCODING more rarely like #361 but don't have a good solution yet)

Previously the end callback was called by checking the state at the end of the READY_STATE_CHANGE event.

The problem with this approach is that there are multiple early exit cases in this function and when they happen the consumer is never informed of the end of the stream.

The new approach is to only dispatch the event when the COMPLETE event is received (Except if an error is dispatched) so the last event is always either 'error' or 'end').

I'd like feedback on this last point as I did the 'only error or end' part by feeling but couldn't find any specification of how it's supposed to work, the other choices being to send error + end when an error is detected.

Previously the end callback was called by checking the state at the end of
the READY_STATE_CHANGE event. The problem with this approach is that there
are multiple early exit cases in this function and when they happen the
consumer is never informed of the end of the stream.

The new approach is to only dispatch the event when the COMPLETE event is
received (Except if an error is dispatched) so the last event is always
either 'error' or 'end').
@stanley-cheung
Copy link
Collaborator

Looks like the require XmlHttp is no longer used anywhere else in the file, please remove it. thanks.

ERROR: /tmpfs/src/github/grpc-web/javascript/net/grpc/web/BUILD.bazel:142:1: Compiling 127 JavaScript files to javascript/net/grpc/web/grpcwebclientbase_test_bin.js failed (Exit 1)
javascript/net/grpc/web/grpcwebclientreadablestream.js:40: ERROR - extra require: 'XmlHttp' is never referenced in this file
const XmlHttp = goog.require('goog.net.XmlHttp');
                ^
  ProTip: "JSC_EXTRA_REQUIRE_WARNING" or "extraRequire" or "legacyGoogScopeRequire" can be added to the `suppress` attribute of:
  //javascript/net/grpc/web:grpcwebclientreadablestream
  Alternatively /** @suppress {legacyGoogScopeRequire} */ can be added to the source file.

1 error(s), 0 warning(s), 97.3% typed

INFO: Elapsed time: 95.875s, Critical Path: 65.77s
INFO: 537 processes: 396 linux-sandbox, 141 worker.
FAILED: Build did NOT complete successfully
//javascript/net/grpc/web:grpcwebclientbase_test                      NO STATUS
//javascript/net/grpc/web:grpcwebstreamparser_test                       PASSED in 1.0s

As to your last question, Yes I believe the early exits is a problem we need to fix. But on the other hand, I am not very sure about when exactly the underlying end event fires either.

Copy link
Collaborator

@stanley-cheung stanley-cheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! I removed the require statement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants