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

http.CloseNotifier is deprecated #347

Closed
wayneashleyberry opened this issue Aug 28, 2018 · 1 comment
Closed

http.CloseNotifier is deprecated #347

wayneashleyberry opened this issue Aug 28, 2018 · 1 comment

Comments

@wayneashleyberry
Copy link

wayneashleyberry commented Aug 28, 2018

middleware/wrap_writer.go:103:38: http.CloseNotifier is deprecated: the CloseNotifier interface predates Go's context package. New code should use Request.Context instead.  (megacheck)
	cn := f.basicWriter.ResponseWriter.(http.CloseNotifier)
	                                    ^
middleware/wrap_writer.go:129:7: http.CloseNotifier is deprecated: the CloseNotifier interface predates Go's context package. New code should use Request.Context instead.  (megacheck)
var _ http.CloseNotifier = &httpFancyWriter{}

Started seeing this pop up in my linters, not sure if this code could simply be removed?


From godoc.org:

Deprecated: the CloseNotifier interface predates Go's context package. New code should use Request.Context instead. — https://golang.org/pkg/net/http/#CloseNotifier

@pkieltyka pkieltyka mentioned this issue Jan 8, 2019
5 tasks
pkieltyka added a commit that referenced this issue Jan 8, 2019
@pkieltyka
Copy link
Member

done in #378

leighmcculloch added a commit to stellar/go that referenced this issue Feb 11, 2020
…2256)

### What

Remove old code that uses a http.ResponseWriter's CloseNotifier channel to cause the request's context to be canceled on the server side.

### Why

I want to upgrade us to chi v4 which no longer supports CloseNotifier and I want to remove any use of it in our code to ensure we aren't dependent on chi providing ResponseWriter's that support it. It turns out the removed code is no longer necessary and can be just deleted without any new functionality. In Go 1.8  the context was changed to automatically cancel if the client closes its connection and so we do not need to also listen to the CloseNotifier channel or replace it with anything else.

As a bonus this also reduces the number of Go routines required to handle each request in Horizon by one.

### References

Go 1.8 Release Notes:

>The Context returned by Request.Context is canceled if the underlying net.Conn closes. For instance, if the user closes their browser in the middle of a slow request, the Handler can now detect that the user is gone. This complements the existing CloseNotifier support.

https://golang.org/doc/go1.8#net_http

The CloseNotifier interface has been deprecated since Go 1.8:

https://golang.org/pkg/net/http/#CloseNotifier

The muxer/router we use, `chi`, removed any functionality relating to CloseNotifier in its major 4.0.0 release:

go-chi/chi#347

### Testing

I wrote a test and confirmed that it fails on Go 1.7 when the code in the httpx package was deleted and passes when it is present. I then ran this test with and without the deleted code in Go 1.8 and Go 1.13 and in both cases it passes, which confirms that this would have been fine to remove once the oldest version of Go we supported was 1.8. I removed the test before merging, but the test is still visible in the commit history of the PR #2256.
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

No branches or pull requests

2 participants