Remove http.CloseNotifier code from Go >= 1.7 builds #795
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
|
I signed it! |
|
CLAs look good, thanks! |
There was a problem hiding this comment.
Hi @SpikesDivZero, thanks for your contribution! I'm not sure how, but it seems like we've ended up with some travis files added in this PR. Travis is no longer used by this project, so can you please strip them out? Thanks!
| @@ -423,6 +424,7 @@ func Register{{$svc.GetName}}{{$.RegisterFuncSuffix}}Client(ctx context.Context, | |||
| } | |||
| }(ctx.Done(), cn.CloseNotify()) | |||
| } | |||
| {{- end }} | |||
There was a problem hiding this comment.
I think we can strip this out unconditionally, there's no need for this to be tied to a flag, it's a deprecated feature.
There was a problem hiding this comment.
I'm not opposed to stripping this entirely, but if we go that route, it seems like we should also completely remove support for Go < 1.7 by removing the UseRequestContext and the CLI flag so we don't end up with any inadvertent leaks/regressions.
I'd elected to go this way at first to minimize the risk of silent regressions with older Go releases, since the existence of this flag seems to imply some degree support for older Go releases.
Thoughts?
There was a problem hiding this comment.
To me the choice of using the request context is still relevant today, since you may want to decouple the cancellation from that of the incoming HTTP requests.
There was a problem hiding this comment.
I see, and entirely fair! I'd not considered that view point. Adjusting.
Codecov Report
@@ Coverage Diff @@
## master #795 +/- ##
=======================================
Coverage 53.39% 53.39%
=======================================
Files 30 30
Lines 3362 3362
=======================================
Hits 1795 1795
Misses 1392 1392
Partials 175 175
Continue to review full report at Codecov.
|
These were auto-generated by the docker image used to generate changes, as advised by the CONTRIBUTING.md (jfbrandhorst/grpc-gateway-build-env). Docker image inspect shows the following relevant details: I'm more than happy to remove those manually, but I suspect they'll reappear for others following the provided instructions. |
That's very curious because the regeneration script should be running in CI and checking for diffs. It looks like swagger-codegen is outputting travis files, as evident by this log: https://circleci.com/gh/grpc-ecosystem/grpc-gateway/144. How is it not failing the |
|
Lets raise an issue about the swagger-codegen stuff and remove it from this PR diff please. We can investigate asynchronously - I know for sure we don't want it in the repo though. |
http.CloseNotifier is deprecated as of Go 1.11, with a note that "the CloseNotifier interface predates Go's context package. New code should use Request.Context instead." Seeing as Request.Context has been around since 1.7, it seems likely safe to adjust our template to omit this block whenever Request.Context is in use.
|
Fixed both issues, re-pushed, and all tests look green. |
|
LGTM, thanks! |
Edit for others reading this later: The behavior changed between when this was originally written and now. See this comment.
--
http.CloseNotifier is deprecated as of Go 1.11, with a note that "the
CloseNotifier interface predates Go's context package. New code should
use Request.Context instead."
Seeing as Request.Context has been around since 1.7, it seems likely
safe to adjust our template to omit this block whenever Request.Context
is in use.
Fixes: #736