Remove http.CloseNotifier code from Go >= 1.7 builds #795

SpikesDivZero · 2018-11-03T20:17:11Z

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

googlebot · 2018-11-03T20:17:14Z

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. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

SpikesDivZero · 2018-11-03T20:19:47Z

I signed it!

googlebot · 2018-11-03T20:19:48Z

CLAs look good, thanks!

johanbrandhorst

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!

johanbrandhorst · 2018-11-03T20:22:41Z

protoc-gen-grpc-gateway/gengateway/template.go

@@ -423,6 +424,7 @@ func Register{{$svc.GetName}}{{$.RegisterFuncSuffix}}Client(ctx context.Context,
 				}
 			}(ctx.Done(), cn.CloseNotify())
 		}
+	{{- end }}


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.

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?

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.

I see, and entirely fair! I'd not considered that view point. Adjusting.

codecov-io · 2018-11-03T20:23:21Z

Codecov Report

Merging #795 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #795   +/-   ##
=======================================
  Coverage   53.39%   53.39%           
=======================================
  Files          30       30           
  Lines        3362     3362           
=======================================
  Hits         1795     1795           
  Misses       1392     1392           
  Partials      175      175

Impacted Files	Coverage Δ
protoc-gen-grpc-gateway/gengateway/template.go	`60.6% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c203e72...d22b116. Read the comment docs.

SpikesDivZero · 2018-11-03T20:26:18Z

I'm not sure how, but it seems like we've ended up with some travis files added in this PR.

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:

    "Id": "sha256:2011172b884689f4c0f4dcbb50d408abb1acccbcd002e0a054e0492dbcce94a8",
    "RepoTags": [
        "jfbrandhorst/grpc-gateway-build-env:latest"
    ],
    "RepoDigests": [
        "jfbrandhorst/grpc-gateway-build-env@sha256:81527758368154998c24d310501e92f8b805987ee164d7050b7be6d6b4deffea"
    ],
    "Created": "2018-10-09T21:58:26.512757165Z",

I'm more than happy to remove those manually, but I suspect they'll reappear for others following the provided instructions.

johanbrandhorst · 2018-11-03T20:31:17Z

I'm not sure how, but it seems like we've ended up with some travis files added in this PR.

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:
    "Id": "sha256:2011172b884689f4c0f4dcbb50d408abb1acccbcd002e0a054e0492dbcce94a8",
    "RepoTags": [
        "jfbrandhorst/grpc-gateway-build-env:latest"
    ],
    "RepoDigests": [
        "jfbrandhorst/grpc-gateway-build-env@sha256:81527758368154998c24d310501e92f8b805987ee164d7050b7be6d6b4deffea"
    ],
    "Created": "2018-10-09T21:58:26.512757165Z",
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 git diff --exit-code?

johanbrandhorst · 2018-11-03T20:33:19Z

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.

SpikesDivZero · 2018-11-03T20:45:16Z

Fixed both issues, re-pushed, and all tests look green.

johanbrandhorst · 2018-11-03T20:46:11Z

LGTM, thanks!

googlebot added the cla: no label Nov 3, 2018

googlebot added cla: yes and removed cla: no labels Nov 3, 2018

johanbrandhorst requested changes Nov 3, 2018

View reviewed changes

johanbrandhorst approved these changes Nov 3, 2018

View reviewed changes

johanbrandhorst merged commit dfdde99 into grpc-ecosystem:master Nov 3, 2018

SpikesDivZero deleted the http-close-notifier branch November 3, 2018 20:46

johanbrandhorst mentioned this pull request Nov 6, 2018

Update or remove swagger-codegen #254

Open

danielqsj mentioned this pull request Apr 16, 2019

Remove http.CloseNotifier kubernetes/kubernetes#76648

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove http.CloseNotifier code from Go >= 1.7 builds #795

Remove http.CloseNotifier code from Go >= 1.7 builds #795

SpikesDivZero commented Nov 3, 2018 •

edited

Loading

googlebot commented Nov 3, 2018

SpikesDivZero commented Nov 3, 2018

googlebot commented Nov 3, 2018

johanbrandhorst left a comment

johanbrandhorst Nov 3, 2018

SpikesDivZero Nov 3, 2018

johanbrandhorst Nov 3, 2018

SpikesDivZero Nov 3, 2018

codecov-io commented Nov 3, 2018 •

edited

Loading

SpikesDivZero commented Nov 3, 2018

johanbrandhorst commented Nov 3, 2018

johanbrandhorst commented Nov 3, 2018

SpikesDivZero commented Nov 3, 2018

johanbrandhorst commented Nov 3, 2018

Remove http.CloseNotifier code from Go >= 1.7 builds #795

Remove http.CloseNotifier code from Go >= 1.7 builds #795

Conversation

SpikesDivZero commented Nov 3, 2018 • edited Loading

googlebot commented Nov 3, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

SpikesDivZero commented Nov 3, 2018

googlebot commented Nov 3, 2018

johanbrandhorst left a comment

Choose a reason for hiding this comment

johanbrandhorst Nov 3, 2018

Choose a reason for hiding this comment

SpikesDivZero Nov 3, 2018

Choose a reason for hiding this comment

johanbrandhorst Nov 3, 2018

Choose a reason for hiding this comment

SpikesDivZero Nov 3, 2018

Choose a reason for hiding this comment

codecov-io commented Nov 3, 2018 • edited Loading

Codecov Report

SpikesDivZero commented Nov 3, 2018

johanbrandhorst commented Nov 3, 2018

johanbrandhorst commented Nov 3, 2018

SpikesDivZero commented Nov 3, 2018

johanbrandhorst commented Nov 3, 2018

SpikesDivZero commented Nov 3, 2018 •

edited

Loading

codecov-io commented Nov 3, 2018 •

edited

Loading