Wishlist bug for what net/http would look like if we could make backwards-incompatible changes. Anything goes. I'll start: * Handler (http://golang.org/pkg/net/http/#Handler) currently takes a pointer to a 208 byte Request struct. * The Request struct (http://golang.org/pkg/net/http/#Request) has all its fields publicly exposed, most of which require memory allocation: -- Method -- URL (itself 104 bytes, with a bunch of strings requiring allocation) -- Header map + slices + strings (even if never accessed) -- TransferEncoding slice (even if not accessed) -- Host string (even if not accessed) -- RemoteAddr string (even if not accessed) -- RequestURI (even if not accessed) -- TLS state struct (even if not accessed) For a lightweight handler that doesn't touch anything (say, serves some static content from memory), this means we can't do any better than generating ~1KB of garbage per request. I'd prefer to make a ServerRequest interface with accessor methods which can generate needed data on demand. This would also simplify our docs on our doubly-abused-in-different-ways *Request, which contains documentation gems like: // PostForm contains the parsed form data from POST or PUT // body parameters. // This field is only available after ParseForm is called. // The HTTP client ignores PostForm and uses Body instead. PostForm url.Values If we had byte views (issue #5376), I would also say that most fields in the ServerRequest, URL, Header, and FormValues are all byte views with validity scoped to the duration of the http.Handler call, instead of strings.
Comment From: nigeltao
A CookieJar's methods should return error, and maybe be renamed from http.CookieJar to cookiejar.Interface.
Comment From: gopherbot
In http.Client, there should be a uniform way of specifying request headers on the initial request and subsequent requests when following a redirect. Current the Request.Header field is useless if the request might get redirected. See issue #5782. Maybe something like a func(*Request) field in the request, that gets called on this request and each redirected one before it gets sent?
Comment From: gopherbot
The fact that headers are parsed into a map is a mega bummer for garbage reduction. If the main way headers were exposed was more opaque, the headers map could be made up of offsets into the buffer that came straight from Read.
Comment From: rsc
Labels changed: added repo-main.
Comment From: nigeltao
Related to comment #1, net/http/cookiejar.PublicSuffixList's PublicSuffix method should perhaps return (string, error) instead of (string), in case the PSL implementation is passed non-canonicalized input such as upper-cased "EXAMPLE.COM.AU". Returning an explicit error is probably safer than silently returning a "" that would mean a liberal cookie policy.
Comment From: alberts
It would be nice to have a handle on when all the goroutines that handle requests have terminated after closing the listener (thereby breaking out of the accept loop) for cases where they are using a shared resource that needs to be cleaned up (e.g. an in-process database that needs to be closed). And maybe a way to force-close all their connections. Especially useful for tests and SIGTERM situations.
Comment From: gopherbot
You can do that by counting open connections returned by Listener's Accept: http://play.golang.org/p/sPmvXaR-2M (Disclaimer: I didn't test that code.) Robert
Comment From: alberts
@Robert thanks, I did something similar in the end.
Comment From: alberts
@Robert For what it's worth, I think your listener breaks in the more general case where you hand off the socket to both a reader and a writer goroutine. There a "done" channel might be better.
Comment From: gopherbot
Summarizing a discussion with fullung@: That version didn't handle multiple calls to Close and had a (likely irrelevant) ReadWrite/Close race condition. The corrected version is here: http://play.golang.org/p/WTKa021ipJ
Comment From: gopherbot
Summarizing a discussion with fullung@: That version didn't handle multiple calls to Close and had a (likely irrelevant) ReadWrite/Close race condition. The corrected version is here: http://play.golang.org/p/WTKa021ipJ
Comment From: rsc
Labels changed: added release-none.
Comment From: gopherbot
We have http.StripPrefix which modifies Request.URL.Path and passes the modified request to a handler. We also have http.Redirect, which assumes that Request.URL.Path is absolute. Thus, if one tries to use http.Redirect within a handler that is wrapped in http.StripPrefix the redirect will point to a wrong URL. In fact, we don't use http.Redirect in http.FileServer for this very reason: https://code.google.com/p/go/source/browse/src/pkg/net/http/fs.go#339 We could abandon StripPrefix and have another way of passing the prefix to handlers. We could also keep the original path and the possibly-shortened path in request, so that redirect could use the original path.
Comment From: gopherbot
Comment 14 by nigel.tao.gnome:
Make http.Server recovering from panics opt-in instead of opt-out. https://groups.google.com/forum/#!msg/golang-dev/rXs4TG1gdXw/7BQ29S4NPrgJ
Comment From: tv42
I wish there was a way to delegate URL path subtrees to another handler in a way where recipient doesn't have to specifically understand the whole path, but can still easily do redirects and such. A convention for "this is the unprocessed part of the path".
For example, try using the same handler for /foo/ and /bar/quux/, having the handler extract the immediate child segment as a numeric ID, doing a database lookup, and creating a redirect to a canonical non-numerically named child (e.g. http://macworld.com/article/2367748 )
Right now, mutating Request.URL.Path (like StripPrefix) destroys knowledge of the requested URL (and re-parsing Request.RequestURI seems wrong).
Comment From: CAFxX
Only partially related (it belongs more in the scheduler/netpoller) but since net/http has ListenAndServe* I'll add it here: integrate support for REUSEPORT in the netpoller (i.e. open multiple listening sockets on the same port -one for proc?- and accept() on multiple procs)
Comment From: pciet
https://github.com/gorilla/websocket uses the Hijacker interface (https://github.com/gorilla/websocket/blob/v1.2.0/server.go#L106).
From the net/http documentation:
The default ResponseWriter for HTTP/1.x connections supports Hijacker, but HTTP/2 connections intentionally do not. ResponseWriter wrappers may also not support Hijacker. Handlers should always test for this ability at runtime.
So it looks like websockets (at least gorilla websockets) can't be upgraded on pure HTTP/2 connections with Go.
Perhaps pull in some of the websocket upgrade logic into net/http? @garyburd
Comment From: bradfitz
So it looks like websockets (at least gorilla websockets) can't be upgraded on pure HTTP/2 connections with Go.
Well, not because of any limitation in Go. The actual reason is there's no spec for WebSockets over HTTP/2. That's why all browsers start a new HTTP/1.1 connection when they want to start WebSockets.
Comment From: pciet
Sorry, I should have done more research into HTTP/2. I've mentioned websockets in Go 2 at https://github.com/golang/go/issues/18152
Comment From: ianlancetaylor
Copying from #21082:
What did you do? Tried to use a custom logger on ReverseProxy.ErrorLog (like logrus)
What did you expect to see? A program compiling fine and logging going through my logger from logrus
What did you see instead? Compilation failed as ReverseProxy.ErrorLog has to be bound to a log.Logger.
ReverseProxy.ErrorLog should accept an interface instead, to allow using any compliant logging system.
Related to : sirupsen/logrus#588
Comment From: RalphCorderoy
Follow RFC 7230's Field Order section: https://tools.ietf.org/html/rfc7230#section-3.2.2 It points out when order can be significant, as well as the separate issue of good practice to allow short-circuiting when parsing.
Comment From: docmerlin
http.ReadRequest should be able to take any io.Reader not just a *bufio.Reader
https://groups.google.com/forum/#!topic/golang-nuts/7ZFA-DWS9KY
Comment From: nhooyr
@ianlancetaylor why can't we change http.ReadRequest to take any io.Reader? How does that break Go 1 compatibility?
Comment From: bradfitz
@nhooyr, it would break code like this: https://play.golang.org/p/Z3HSnulUDJR
(not uncommon in tests)
Comment From: docmerlin
The compiler auto converting/wrapping return types to the proper accepted interface would fix this problem, and make code a lot cleaner. On Mon, May 7, 2018 at 12:24 PM Brad Fitzpatrick notifications@github.com wrote:
@nhooyr https://github.com/nhooyr, it would break code like this: https://play.golang.org/p/Z3HSnulUDJR
(not uncommon in tests)
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/5465#issuecomment-387138655, or mute the thread https://github.com/notifications/unsubscribe-auth/ABLfW9ZIUSf6ahvGNY85d4EVlj5PEq9Kks5twIM6gaJpZM4F0slU .
Comment From: bradfitz
@docmerlin, the compiler implements the language spec, so you're proposing a language change, which is way out of scope for this particular bug.
Comment From: docmerlin
Right, but if we are talking about 2.0, I thought language changes would be in scope? On Mon, May 7, 2018 at 12:30 PM Brad Fitzpatrick notifications@github.com wrote:
@docmerlin https://github.com/docmerlin, the compiler implements the language spec, so you're proposing a language change, which is way out of scope for this particular bug.
— You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/5465#issuecomment-387140450, or mute the thread https://github.com/notifications/unsubscribe-auth/ABLfW8hPopQlDMgUp3Vk_RxuyIemeG8Pks5twISngaJpZM4F0slU .
Comment From: bradfitz
@docmerlin, perhaps, but language changes have a high bar. They need their own bug and a large write-up and discussion, not just a casual aside on some library issue. Any language changes that do happen in Go 2 will of course affect the standard library.
Comment From: docmerlin
@bradfitz Alright, thats fair. I'll make an issue for it (the language change). I think that this sort of change to the language would allow these sorts of fixes to be compatible with existing code and make code that relies a lot on closures a lot cleaner.
Comment From: nhooyr
It'd be neat if we could just use *httputil.ReverseProxy and support WebSocket connection upgrades transparently.
Comment From: bradfitz
@nhooyr, there's no reason that needs to wait for Go 2. I'll do it for Go 1.12. I filed #26937.
Comment From: nhooyr
Awesome! I figured we'd need an API change to do this correctly as we have to hijack the client connection. Curious to see how you managed to do it.
Comment From: nhooyr
Nvm, I see now. We just don't use the transport.
Comment From: SelfDrivingCarp
The http/Server.ErrorLog is *log.Logger.
If you want to log server errors in a custom way you have to create a custom io.Writer that assumes each call to Write() is a single, complete log message or you have to buffer, parse, and restructure a stream of calls to Write().
It would be much more flexible to have ErrorLog be an interface such as:
type ErrorLog interface {
Printf(string, ...interface{})
}
Comment From: bradfitz
@AgentZombie, there's another bug open about rethinking log/log interfaces. Whatever we did across std would also be done in net/http.
Comment From: mklencke
How about explicit context support?
ServeHTTP(context.Context, ResponseWriter, *Request)
and then get rid of (*Request).Context(). That way, middleware can also more easily augment the context without suffering #28737.
Comment From: nhooyr
Would be nice to rename http.Header to http.Headers as it is a map of headers and not a single header.
Comment From: RalphCorderoy
Hi @nhooyr, but Header["foo"] gives the single Foo header so it reads okay. I agree another style is to use a plural for an array or map so it really depends on the normal style within http and stdlib as a whole.
Comment From: nhooyr
It’s not the worst thing but most usages of maps and array are plural in the stdlib.
Comment From: shinny-chengzhi
The auto recovering of http.Server didn't generate core dump even if GOTRACEBACK=crash, which make it hard to debug. All the information I get is the call stack which is not enough as I need to examine the value of various variable.
Comment From: guyskk
Consider http client DO NOT following redirects by default. FYI: https://github.com/encode/httpx/discussions/1785
Comment From: bcmills
One more thought via #52727. Especially now that we have errors.Is, a new net/http client should produce errors with more structure.
For example, on a Content-Length mismatch the current client returns a bare io.ErrUnexpectedEOF, rather than (say) a more structured error in the style produced by the os package: perhaps one that includes the URL and/or the expected Content-Length.
Comment From: jchadwick-buf
One thought I have: it seems like it's somewhat common to forget to implement http.Flusher when wrapping an http.ResponseWriter, which can cause some really nasty issues. Would it perhaps be better if the Flush method was simply on http.ResponseWriter, and canonically a no-op if the underlying implementation is unbuffered?
Comment From: rbqvq
A Good News.
If PR #74676 will be accepted, We can use custom tls stack in Go 1.0.