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

cntrib/google.golang.org/grpc: fix overwriting outgoing context #327

Closed
wants to merge 1 commit into from
Closed

cntrib/google.golang.org/grpc: fix overwriting outgoing context #327

wants to merge 1 commit into from

Conversation

osamingo
Copy link

@osamingo osamingo commented Sep 7, 2018

Fixed a bug where the previously set context value will be overwritten.
Also, official docs recommended uses metadata.AppendToOutgoingContext function.

ref. https://github.com/grpc/grpc-go/blob/master/Documentation/grpc-metadata.md#sending-metadata

@dd-caleb
Copy link
Contributor

dd-caleb commented Sep 7, 2018

Thank you for your contribution.

I think the issue may be that we're calling:

md, ok := metadata.FromIncomingContext(ctx)

When we should be calling

md, ok := metadata.FromOutgoingContext(ctx)

Though your solution also probably fixes it. I will take a look later today and add a unit test.

@osamingo
Copy link
Author

osamingo commented Sep 7, 2018

Thanks for your comment!

I will take a look later today and add a unit test.

OK, I'll be waiting for it.

@osamingo
Copy link
Author

osamingo commented Sep 7, 2018

I changed to use metadata.FromOutgoingContext function.

@dd-caleb
Copy link
Contributor

dd-caleb commented Sep 7, 2018

Thanks @osamingo. If you wanted to try write the unit test feel free. Otherwise I'll do it in a few hours and merge this PR.

I think its a pretty critical bug since it breaks anything using outgoing metadata, so I'll try to make a release too.

Thanks again.

@dd-caleb
Copy link
Contributor

dd-caleb commented Sep 7, 2018

@osamingo this change has been released as tag v1.2.3. Thanks again for your help!

@osamingo osamingo deleted the fix-overwriting-outgoing-ctx branch September 7, 2018 18:02
@osamingo
Copy link
Author

osamingo commented Sep 7, 2018

@dd-caleb Great, Thank you for a quick response!

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

Successfully merging this pull request may close these issues.

None yet

2 participants