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

use the request context with net/http handler #1696

Merged
merged 2 commits into from
Nov 30, 2017

Conversation

groob
Copy link
Contributor

@groob groob commented Nov 28, 2017

When the grpc server is used as a net/http handler the request context is being discarded. This breaks logging and other uses where grpc and http are served on the same port in a Go application.

This PR adds the request context to the server transport. I see that grpc is maintaining compatibility down to 1.6 so I'm happy to make the context changes backwards compatible. Looking to get some initial feedback first.

@thelinuxfoundation
Copy link

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
The Linux Foundation CLA GitHub bot

@groob
Copy link
Contributor Author

groob commented Nov 28, 2017

CLA signed.

@menghanl
Copy link
Contributor

This doesn't work in go1.6 because Context() doesn't exist. We had similar problems like this, so there are go16.go and go17.go.

One solution would be to add a contextFromRequest() function, which returns background for go1.6, and req.Context() for >=go1.7.

@groob
Copy link
Contributor Author

groob commented Nov 30, 2017

Yes, @menghanl I'm willing to update the PR to make it compatible with 1.6+ by adding that function. That's the feedback I was looking for :)

@groob
Copy link
Contributor Author

groob commented Nov 30, 2017

Updated to account for 1.6 vs 1.7+

Copy link
Contributor

@menghanl menghanl 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 fix.
LGTM.

@menghanl menghanl merged commit a4bf341 into grpc:master Nov 30, 2017
@dfawley dfawley added this to the 1.9 Release milestone Jan 2, 2018
@groob groob deleted the use_request_context branch March 14, 2018 11:24
@lock lock bot locked as resolved and limited conversation to collaborators Sep 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants