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

proposal: net/http: thoughts for Go 2.0 #5465

Open
bradfitz opened this Issue May 14, 2013 · 35 comments

Comments

Projects
None yet
@bradfitz
Member

bradfitz commented May 14, 2013

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.
@nigeltao

This comment has been minimized.

Contributor

nigeltao commented Jun 4, 2013

Comment 1:

A CookieJar's methods should return error, and maybe be renamed from http.CookieJar to
cookiejar.Interface.
@gopherbot

This comment has been minimized.

gopherbot commented Jun 26, 2013

Comment 2 by robryk:

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?
@gopherbot

This comment has been minimized.

gopherbot commented Oct 9, 2013

Comment 3 by jeff.allen:

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.
@rsc

This comment has been minimized.

Contributor

rsc commented Dec 4, 2013

Comment 4:

Labels changed: added repo-main.

@nigeltao

This comment has been minimized.

Contributor

nigeltao commented Jan 9, 2014

Comment 5:

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.
@alberts

This comment has been minimized.

Contributor

alberts commented Feb 15, 2014

Comment 6:

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.
@gopherbot

This comment has been minimized.

gopherbot commented Feb 15, 2014

Comment 7 by robryk:

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
@alberts

This comment has been minimized.

Contributor

alberts commented Feb 15, 2014

Comment 8:

@Robert thanks, I did something similar in the end.
@alberts

This comment has been minimized.

Contributor

alberts commented Feb 15, 2014

Comment 9:

@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.
@gopherbot

This comment has been minimized.

gopherbot commented Feb 15, 2014

Comment 10 by robryk:

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
@gopherbot

This comment has been minimized.

gopherbot commented Feb 15, 2014

Comment 11 by robryk:

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
@rsc

This comment has been minimized.

Contributor

rsc commented Mar 3, 2014

Comment 12:

Labels changed: added release-none.

@gopherbot

This comment has been minimized.

gopherbot commented Jun 3, 2014

Comment 13 by robryk:

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.
@gopherbot

This comment has been minimized.

gopherbot commented Aug 27, 2014

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

@bradfitz bradfitz self-assigned this Aug 27, 2014

@bradfitz bradfitz removed their assignment Dec 13, 2014

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

@rsc rsc removed release-none labels Apr 10, 2015

@tv42

This comment has been minimized.

tv42 commented Aug 30, 2015

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).

@CAFxX

This comment has been minimized.

Contributor

CAFxX commented Dec 15, 2015

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)

@pciet

This comment has been minimized.

Contributor

pciet commented Dec 21, 2017

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

@bradfitz

This comment has been minimized.

Member

bradfitz commented Dec 21, 2017

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.

@pciet

This comment has been minimized.

Contributor

pciet commented Dec 25, 2017

Sorry, I should have done more research into HTTP/2. I've mentioned websockets in Go 2 at #18152

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 27, 2018

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

@RalphCorderoy

This comment has been minimized.

RalphCorderoy commented Apr 10, 2018

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.

@docmerlin

This comment has been minimized.

docmerlin commented May 7, 2018

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

@nhooyr

This comment has been minimized.

Contributor

nhooyr commented May 7, 2018

@ianlancetaylor why can't we change http.ReadRequest to take any io.Reader? How does that break Go 1 compatibility?

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 7, 2018

@nhooyr, it would break code like this: https://play.golang.org/p/Z3HSnulUDJR

(not uncommon in tests)

@docmerlin

This comment has been minimized.

docmerlin commented May 7, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 7, 2018

@docmerlin, the compiler implements the language spec, so you're proposing a language change, which is way out of scope for this particular bug.

@docmerlin

This comment has been minimized.

docmerlin commented May 7, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 7, 2018

@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.

@docmerlin

This comment has been minimized.

docmerlin commented May 7, 2018

@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.

@nhooyr

This comment has been minimized.

Contributor

nhooyr commented Aug 12, 2018

It'd be neat if we could just use *httputil.ReverseProxy and support WebSocket connection upgrades transparently.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Aug 12, 2018

@nhooyr, there's no reason that needs to wait for Go 2. I'll do it for Go 1.12. I filed #26937.

@nhooyr

This comment has been minimized.

Contributor

nhooyr commented Aug 12, 2018

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.

@nhooyr

This comment has been minimized.

Contributor

nhooyr commented Aug 12, 2018

Nvm, I see now. We just don't use the transport.

@AgentZombie

This comment has been minimized.

AgentZombie commented Aug 20, 2018

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{})
}
@bradfitz

This comment has been minimized.

Member

bradfitz commented Aug 21, 2018

@AgentZombie, there's another bug open about rethinking log/log interfaces. Whatever we did across std would also be done in net/http.

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