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.

Sign up for GitHub

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

Jump to bottom

Fix Grpc-Message decoding #117

Merged
merged 2 commits into from
Apr 21, 2018
Merged

Fix Grpc-Message decoding #117

merged 2 commits into from
Apr 21, 2018

Conversation

ishitatsuyuki
Copy link
Contributor

No description provided.

@ishitatsuyuki
Copy link
Contributor Author

messages are turned into string | undefined due to the old code implicitly violating the contract. Making them null is a better practice though, but as it's backward incompatible some discussion is required.

@ishitatsuyuki
Copy link
Contributor Author

CI green.

@ishitatsuyuki
Copy link
Contributor Author

Review ping.

easyCZ
easyCZ suggested changes Jan 13, 2018
View reviewed changes
Copy link
Contributor

@easyCZ easyCZ 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 PR! Just a few points

ts/src/grpc.ts Outdated
@@ -231,7 +235,7 @@ export namespace grpc {
const gRPCMessage = headers.get("grpc-message") || [];
props.debug && debug("onHeaders.gRPCMessage", gRPCMessage);
if (code !== Code.OK) {
rawOnError(code, gRPCMessage[0]);
rawOnError(code, gRPCMessage.length > 0 ? decodeURIComponent(gRPCMessage[0]) : undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

decodeURIComponent can throw URIError, we should attempt to handle it. Could we also use the "" (empty string) as the value rather than undefined?

@ishitatsuyuki
Copy link
Contributor Author

I have reorganized the code to satisfy the requirements. I tried to minimize code duplication, but basically we have to return early if the decode errors and that part cannot be put inside a function.

@ishitatsuyuki
Copy link
Contributor Author

Oh, ****, the tests are full of assert.equal to undefined.

@ishitatsuyuki
Copy link
Contributor Author

This is basically done, but I have no idea why Edge tests are failing. Can you run a retry?

@ishitatsuyuki
Copy link
Contributor Author

The Edge tests seem to be flaky, and I'm probably out of luck. Can you retry again?

@easyCZ
Copy link
Contributor

easyCZ commented Mar 31, 2018

@ishitatsuyuki Sorry for the delay on this. Would you mind rebasing the PR?

@ishitatsuyuki
Copy link
Contributor Author

Sure, I'll do it tomorrow.

@ishitatsuyuki ishitatsuyuki force-pushed the percent-enc branch 3 times, most recently from 18c6dd6 to ce34f88 Compare April 1, 2018 03:32
@ishitatsuyuki
Copy link
Contributor Author

@easyCZ I think this is ready for review.

easyCZ
easyCZ suggested changes Apr 2, 2018
View reviewed changes
Copy link
Contributor

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

After the rebase there's now a name variable which does not exist anymore

test/ts/src/client.spec.ts Outdated
@@ -351,6 +351,39 @@ describe(`client`, () => {
});
});

headerTrailerCombos((withHeaders, withTrailers) => {
it("should report status code in Unicode for error with headers + trailers" + name, (done) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name variable is not defined.

@ishitatsuyuki
Copy link
Contributor Author

All green, please review again.

jonnyreeves
jonnyreeves reviewed Apr 3, 2018
View reviewed changes
ts/src/client.ts Outdated
try {
statusMessage = this.decodeGRPCStatus(gRPCMessage[0]);
} catch (err) {
this.rawOnError(Code.Internal, "Cannot decode gRPC-Message header");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the correct error code? Should it be unknown as it is an error on the transport library rather than the end users code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's unspecified; maybe it should be Unknown but anyway that only happens when a corrupted/broken message is received. Any reference?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only from the discussion in #150

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aligned the behavior with grpc-go.

@jonnyreeves
Copy link
Contributor

LGTM.

@easyCZ easyCZ merged commit 615bdb7 into improbable-eng:master Apr 21, 2018
easyCZ pushed a commit that referenced this pull request Apr 21, 2018
Revert "Fix Grpc-Message decoding (#117)"
406c20a 
This reverts commit 615bdb7.
@easyCZ easyCZ mentioned this pull request Apr 21, 2018
Merged
easyCZ added a commit that referenced this pull request Apr 21, 2018
@easyCZ
Revert "Fix Grpc-Message decoding (#117)" (#177)
9df9c0a 
This reverts commit 615bdb7.
easyCZ pushed a commit that referenced this pull request Apr 21, 2018
Re-add "Fix Grpc-Message decoding (#117)"
3e69a56 
This reverts commit 9df9c0a.
@easyCZ easyCZ mentioned this pull request Apr 21, 2018
Merged
MarcusLongmuir pushed a commit that referenced this pull request May 1, 2018
@easyCZ @MarcusLongmuir
Re-add "Fix Grpc-Message decoding (#117)" (#178)
81ebe17 
* Re-add "Fix Grpc-Message decoding (#117)"

This reverts commit 9df9c0a.

* Fixed clientWebsocket tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonnyreeves jonnyreeves jonnyreeves left review comments

@easyCZ easyCZ easyCZ requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ishitatsuyuki @easyCZ @jonnyreeves