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

grpc-js-core: prevent callback before 'end' event and handle eos headers as trailers #132

Merged
merged 2 commits into from
Jan 9, 2018

Conversation

kjin
Copy link
Contributor

@kjin kjin commented Dec 20, 2017

grpc-js-core: in unary calls, don't invoke callback before end event

For unary response calls, 'status' might be emitted too soon, and the user won't see any data in their callback. This change ensures that if 'status comes before 'end', that 'end' will invoke the user callback instead.

grpc-js-core: handle eos headers as trailers

We should treat headers with END_STREAM flags as if they were trailers. In addition, I've added code to prevent race conditions for emitted StatusObject instances: if a header or trailer (in that order of priority) is still being put through the filter stack, wait to give those execution threads an opportunity to end the call first, before allowing the call to be ended synchronously in a streamClosed event.

@kjin kjin force-pushed the grpc-js-core-work-3 branch from ad8eef0 to 22438ae Compare December 20, 2017 23:54
@kjin
Copy link
Contributor Author

kjin commented Dec 21, 2017

Just to summarize what we talked about earlier in person -- there are three new await * calls:

  1. Just before possibly emitting status from trailers, wait for headers to finish processing (if they are being processed)
  2. Just before possibly emitting status from 'close' event, wait for trailers to finish processing (if they are being processed)
  3. Just before possibly emitting status from cancelWithStatus call, wait for trailers to finish processing (if they are being processed)

We decided that 1 & 2 are necessary. For 3 I believe this is also necessary because we don't want trailing metadata to be ignored in a race condition where cancelWithStatus could be called before the async processing of trailing metadata is finished. In practice this manifests in a test like this:
https://github.com/grpc/grpc-node/blob/master/test/api/surface_test.js#L905-L913
and also other tests that currently check the exact error type.

FYI, I made two more changes:

  • dropped the change (that looked like this) to call the unary response callback in a call's 'end' handler if it occurs after 'status', per our earlier discussion. It turns out it's not even needed to pass tests.
  • changed 'streamClosed' to 'close', as the event name changed in Node 8.9.3/9.3.0

@kjin kjin merged commit d9d9997 into grpc:master Jan 9, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants