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

Go1.11: http.CloseNotifier is deprecated #736

Closed
SpikesDivZero opened this issue Aug 26, 2018 · 7 comments
Closed

Go1.11: http.CloseNotifier is deprecated #736

SpikesDivZero opened this issue Aug 26, 2018 · 7 comments

Comments

@SpikesDivZero
Copy link
Contributor

Hi all,

I was checking if we could upgrade our project to Go 1.11 now that it's released, and one of our static analysis tools is erring with "http.CloseNotifier is deprecated". I tried upgrading my binaries locally, and then checked the git repo and see that it's still present in template.go on the master branch.

Could the template be upgraded so that we're no longer relying on deprecated code? (I'll admit that I don't know what the GRPC-Gateway compatibility promise is, and if such an upgrade would break that.)

Thanks,
-Spikes

@johanbrandhorst
Copy link
Collaborator

I think this should be a reasonably easy first PR for anyone wanting to do it - the CloseNotifier code is only there for when the request context isn't available. Feel free to submit a PR with it removed :).

@eundoosong
Copy link

@johanbrandhorst I would like to work on this issue.
Does this mean below CloseNotifier should be removed?

    if cn, ok := w.(http.CloseNotifier); ok {
      go func(done <-chan struct{}, closed <-chan bool) {
        select {
        case <-done:
        case <-closed:
          cancel()
        }
      }(ctx.Done(), cn.CloseNotify())
    }

@johanbrandhorst
Copy link
Collaborator

Yeah, we can safely remove that whole if-clause I think.

@maros7
Copy link

maros7 commented Oct 16, 2018

@srikrsna are you planning on finishing the PR #752 related to this issue? Or would it be okay if I finished it?

@johanbrandhorst
Copy link
Collaborator

@maros7 before you do anything, I suggest waiting for #777 to be merged, it should be imminently. It will remove any CI issues we had.

@johanbrandhorst
Copy link
Collaborator

I intend to bump all the open PRs once that is in master

@johanbrandhorst
Copy link
Collaborator

This is a great first issue for anyone looking for an easy contribution!

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

No branches or pull requests

4 participants