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

Add a ServeHTTP method to *grpc.Server #514

Merged
merged 1 commit into from Feb 12, 2016

Conversation

Projects
None yet
9 participants
@bradfitz
Copy link
Contributor

bradfitz commented Feb 1, 2016

This adds new http.Handler-based ServerTransport in the process,
reusing the HTTP/2 server code in x/net/http2 or Go 1.6+.

All end2end tests pass with this new ServerTransport.

Fixes #75

Also:
Updates #495 (lets user fix it with middleware in front)
Updates #468 (x/net/http2 validates)
Updates #147 (possible with x/net/http2)
Updates #104 (x/net/http2 does this)

@iamqizhao

This comment has been minimized.

Copy link
Contributor

iamqizhao commented Feb 1, 2016

Some errors on travis:

go test -v -race -cpu 1,4 google.golang.org/grpc/...
=== RUN TestRetry-4
2016/02/01 06:41:51 transport: http2Client.notifyError got notified that the client transport was broken read tcp 127.0.0.1:48304: connection reset by peer.
2016/02/01 06:41:55 transport: http2Client.notifyError got notified that the client transport was broken EOF.
2016/02/01 06:42:07 transport: http2Client.notifyError got notified that the client transport was broken EOF.
FAIL google.golang.org/grpc/test 134.489s
? google.golang.org/grpc/test/codec_perf [no test files]
? google.golang.org/grpc/test/grpc_testing [no test files]

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

bradfitz commented Feb 1, 2016

@iamqizhao, yeah, I saw that. I can't reproduce yet locally. I'm trying to figure out what's different between travis and my machines.

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

bradfitz commented Feb 1, 2016

@iamqizhao, also, all these tests are way too noisy with expected log spam mixed in with real errors. We need to quiet these tests up so real problems are easy to see.

@iamqizhao

This comment has been minimized.

Copy link
Contributor

iamqizhao commented Feb 1, 2016

My experience with travis is that its VM/container is much slower than our desktop so that it sometimes can expose some races which are hard to show on our desktops.

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

bradfitz commented Feb 1, 2016

The problem with this test is that it creates more goroutines than the race detector is capable of tracking.

I will shorten this test when running in race mode.

@bradfitz bradfitz force-pushed the bradfitz:servehttp branch from a4a61af to 20d0619 Feb 1, 2016

@googlebot

This comment has been minimized.

Copy link

googlebot commented Feb 1, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@bradfitz bradfitz force-pushed the bradfitz:servehttp branch from 20d0619 to 85d0357 Feb 1, 2016

@googlebot

This comment has been minimized.

Copy link

googlebot commented Feb 1, 2016

CLAs look good, thanks!

if *httpMode {
// Running a gRPC server when you need to integrate with an existing
// net/http server on the same port: (using HTTP routing as needed)
http.Handle("/", s)

This comment has been minimized.

@tamird

tamird Feb 2, 2016

Contributor

could you have one of the examples listen on a path that isn't "/"? It's not clear to me what client changes would be required.

This comment has been minimized.

@bradfitz

bradfitz Feb 2, 2016

Author Contributor

I don't think gRPC the protocol supports such a thing, so client libraries don't have a way to do it.

Where this is more useful is you're now able to use an net/http.Handler-based router that routes based on the content-type of requests, routing gRPC in one direction (even sub-muxing on service or path), and other stuff to your main website.

This comment has been minimized.

@tamird

tamird Feb 2, 2016

Contributor

Understood. In CockroachDB we're incrementally migrating from net/rpc to grpc - looks like net/rpc already gets out of the way by using a DefaultRPCPath and grpc sets a useful content-type. Seems like that might be enough. Thanks!

This comment has been minimized.

@bradfitz

bradfitz Feb 2, 2016

Author Contributor

In CockroachDB we're incrementally migrating from net/rpc to grpc

Nice!

This comment has been minimized.

@tamird

tamird Feb 2, 2016

Contributor

Having some trouble with this - it doesn't seem that the content type is coming through on grpc-initiated requests.

Having applied this diff to grpc (on top of this PR):

diff --git a/server.go b/server.go
index 577c031..276b98e 100644
--- a/server.go
+++ b/server.go
@@ -438,6 +438,7 @@ func (hl *handlerListener) authenticateConn(rawConn net.Conn) {
 func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
    st, err := transport.NewServerHandlerTransport(w, r)
    if err != nil {
+       grpclog.Printf("error: %s", err)
        http.Error(w, err.Error(), http.StatusInternalServerError)
        return
    }

I see many instances of error: transport: invalid request content-type. Having inspected the request headers myself, I can tell you they're empty. Any ideas what might be going on here?

EDIT: go 1.5, using http2.ConfigureServer(&httpServer, nil) for http2 support.

This comment has been minimized.

@tamird

tamird Feb 2, 2016

Contributor

I believe so - note that NewServerHandlerTransport first checks the r.ProtoMajor and then the content-type, so r.ProtoMajor must be 2.

This comment has been minimized.

@bradfitz

bradfitz Feb 2, 2016

Author Contributor

@tamird, I modified the greeter_server/main.go like:

                http.Handle("/", myMux{s})
...
type myMux struct {
        gs *grpc.Server
}

func (m myMux) ServeHTTP(w http.ResponseWriter, r *http.Request) {
        log.Printf("Got request: %#v", r)
        m.gs.ServeHTTP(w, r)
}

And hit it with greeter_client and got this output logged:

2016/02/02 05:19:06 Running hello server on localhost:50051 ...
2016/02/02 05:19:10 Got request: &http.Request{Method:"POST", URL:(*url.URL)(0xc820278300), Proto:"HTTP/2.0", ProtoMajor:2, ProtoMinor:0, Header:http.Header{"Content-Type":[]string{"application/grpc"}, "User-Agent":[]string{"grpc-go/0.11"}, "Te":[]string{"trailers"}}, Body:(*http.http2requestBody)(0xc820270810), ContentLength:-1, TransferEncoding:[]string(nil), Close:false, Host:"localhost", Form:url.Values(nil), PostForm:url.Values(nil), MultipartForm:(*multipart.Form)(nil), Trailer:http.Header(nil), RemoteAddr:"127.0.0.1:43332", RequestURI:"/helloworld.Greeter/SayHello", TLS:(*tls.ConnectionState)(0xc820244d10), Cancel:(<-chan struct {})(nil)}

Seems to be working for me. That was with Go 1.6. I also tried with Go 1.5, which required that I changed the example code to:

    var httpServer http.Server
    httpServer.Handler = myMux{s}
    httpServer.Addr = *listen
    http2.ConfigureServer(&httpServer, nil)
    log.Fatal(httpServer.ListenAndServeTLS(file("server1.pem"), file("server1.key")))

... and then it logged the same output using Go 1.5.

What is your code?

This comment has been minimized.

@tamird

tamird Feb 2, 2016

Contributor

My code looks roughly the same:

    rpcServer := rpc.NewServer(nodeRPCContext)
    grpcServer := grpc.NewServer()
    handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        log.Printf("Got request: %#v", r)
        if r.ProtoMajor == 2 && strings.Contains(r.Header.Get("Content-Type"), "application/grpc") {
            grpcServer.ServeHTTP(w, r)
        } else {
            rpcServer.ServeHTTP(w, r)
        }
    })

    ln, err := listen(addr, nodeRPCContext.GetServerTLSConfig()) // ends up calling tls.NewListener
    if err != nil {
        return nil, err
    }

    var mu sync.Mutex
    activeConns := make(map[net.Conn]struct{})

    httpServer := http.Server{
        Handler: handler,
        ConnState: func(conn net.Conn, state http.ConnState) {
            mu.Lock()
            switch state {
            case http.StateNew:
                activeConns[conn] = struct{}{}
            case http.StateClosed:
                delete(activeConns, conn)
            }
            mu.Unlock()
        },
    }
    if err := http2.ConfigureServer(&httpServer, nil); err != nil {
        return nil, err
    }

    log.Fatal(httpServer.Serve(ln))

Here's what that Printf line logs:

Got request: &http.Request{Method:"PRI", URL:(*url.URL)(0xc8204b8100), Proto:"HTTP/2.0", ProtoMajor:2, ProtoMinor:0, Header:http.Header{}, Body:(*struct { http.eofReaderWithWriteTo; io.Closer })(0x59dcf10), ContentLength:0, TransferEncoding:[]string(nil), Close:false, Host:"", Form:url.Values(nil), PostForm:url.Values(nil), MultipartForm:(*multipart.Form)(nil), Trailer:http.Header(nil), RemoteAddr:"127.0.0.1:51108", RequestURI:"*", TLS:(*tls.ConnectionState)(0xc8205b02c0), Cancel:(<-chan struct {})(nil)}

Note that this is a bidirectional streaming RPC, not sure if it matters.

The code is here: https://github.com/cockroachdb/cockroach/blob/gossip-grpc/server/node_test.go#L68 but it may be quite hard to understand how everything works :(

This comment has been minimized.

@tamird

tamird Feb 2, 2016

Contributor

Found it. We are using a custom net.Listener implementation, and hitting this: https://github.com/golang/go/blob/release-branch.go1.5/src/net/http/server.go#L1296

See also golang/go#12737.

EDIT: nope, that was obviously wrong. Still stumped. I've edited my standard library to add some logging:

diff --git a/src/net/http/server.go b/src/net/http/server.go
index a3e4355..834a1b6 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -1293,7 +1293,9 @@ func (c *conn) serve() {
        }
    }()

+   log.Printf("type assertion")
    if tlsConn, ok := c.rwc.(*tls.Conn); ok {
+       log.Printf("type assertion ok")
        if d := c.server.ReadTimeout; d != 0 {
            c.rwc.SetReadDeadline(time.Now().Add(d))
        }
@@ -1306,6 +1308,8 @@ func (c *conn) serve() {
        }
        c.tlsState = new(tls.ConnectionState)
        *c.tlsState = tlsConn.ConnectionState()
+       log.Printf("proto: %s", c.tlsState.NegotiatedProtocol)
+       log.Printf("tlsnNextProto map: %s", c.server.TLSNextProto)
        if proto := c.tlsState.NegotiatedProtocol; validNPN(proto) {
            if fn := c.server.TLSNextProto[proto]; fn != nil {
                h := initNPNRequest{tlsConn, serverHandler{c.server}}

This produces:

I0202 06:40:42.176913 82232 server.go:1296  type assertion
I0202 06:40:42.176923 82232 server.go:1298  type assertion ok
I0202 06:40:42.186413 82232 server.go:1311  proto:
I0202 06:40:42.186444 82232 server.go:1312  tlsnNextProto map: map[h2:%!!(MISSING)s(func(*http.Server, *tls.Conn, http.Handler)=0x4662ad0) h2-14:%!!(MISSING)s(func(*http.Server, *tls.Conn, http.Handler)=0x4662ad0)]

Somehow NegotiatedProtocol is not being set. Will continue investingating.

This comment has been minimized.

@tamird

tamird Feb 2, 2016

Contributor

Ah, turns out this is because http2.ConfigureServer mutates server.TLSConfig, which is not the config we're using to create our listener. Initializing our server.TLSConfig to our config before the call to ConfigureServer makes this work!

server.go Outdated
@@ -602,13 +752,18 @@ func (s *Server) Stop() {
s.lis = nil
cs := s.conns
s.conns = nil
for c := range s.hconns {
go c.Close()

This comment has been minimized.

@tamird

tamird Feb 2, 2016

Contributor

is it necessary to spin a goroutine for each connection? in fact, shouldn't this be a blocking call?

This comment has been minimized.

@bradfitz

bradfitz Feb 2, 2016

Author Contributor

Some net.Conn impls block unreasonably long on Close. Until Go 1.6, even Go's *tls.Conn could hang forever in Close if there was a blocking Write in-flight (which wouldn't be woken up by the Close, since the Close was previously blocked forever on a mutex)

Spinning up a goroutine is nearly free.

This comment has been minimized.

@tamird

tamird Feb 2, 2016

Contributor

Doesn't doing this asynchronously violate least surprise for this function? It may return before all connections are closed.

This comment has been minimized.

@bradfitz

bradfitz Feb 2, 2016

Author Contributor

Surprising who? This code is only used by tests, and the tests don't synchronize it with anything else.

Who else is using this? grpc-go really needs a graceful shutdown mechanism, and this isn't it (or at least not yet). I don't want to fix that in this bug, but Stop by itself is pretty useless today. It's mostly a cleanup mechanism for tests, since normal servers don't call Stop. They'd really want graceful shutdown if they know they're going to shut down (else they can just crash/exit).

This comment has been minimized.

@tamird

tamird Feb 2, 2016

Contributor

Fair enough.

This comment has been minimized.

@iamqizhao

iamqizhao Feb 5, 2016

Contributor

I would prefer the same treatment like s.conns. It does not matter how long it takes Stop to return as long as it does not hold the lock.

We do need a graceful shutdown (issue#147). But Stop() is not a method for testing only. Some service providers may want to stop a grpc server immediately but they do not want to crash/exit the entire process (there are other modules working for other purposes).

This comment has been minimized.

@maniksurtani

maniksurtani Feb 5, 2016

+1 to a proper Stop() and not relying on a crash/exit. But maybe that should be a separate PR.

@@ -47,9 +49,19 @@ const (
defaultName = "world"
)

var withInsecureTLS = flag.Bool("insecure_tls", false, "Use Insecure TLS; suitable for hitting the greeter_server in -use_http mode")

This comment has been minimized.

@iamqizhao

iamqizhao Feb 4, 2016

Contributor

why do we need to skip verify when using -use_http mode?

This comment has been minimized.

@bradfitz

bradfitz Feb 4, 2016

Author Contributor

Because I only implemented ServeHTTP for TLS. That's all Go's http2 currently does.

This comment has been minimized.

@bradfitz

bradfitz Feb 4, 2016

Author Contributor

Oh, you mean I should use the root CA option? I remember seeing that somewhere.

I could do that.

@iamqizhao

This comment has been minimized.

Copy link
Contributor

iamqizhao commented Feb 4, 2016

Can you put the greeter change into a separate pull request?

And can you add a http server listening on the same port and a http client in the greeter client? I think after that the greeter is a much better example to show how to use the feature introduced by the new ServeHTTP.

Thank you.

@bradfitz bradfitz force-pushed the bradfitz:servehttp branch from 85d0357 to 31ff71d Feb 4, 2016

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

bradfitz commented Feb 4, 2016

Done.

Removed example changes for now. They can come in a separate change later.

@bradfitz bradfitz force-pushed the bradfitz:servehttp branch from 31ff71d to eeff984 Feb 4, 2016

server.go Outdated
}
}

type handlerListener struct {

This comment has been minimized.

@iamqizhao

iamqizhao Feb 4, 2016

Contributor

Add some comments to this struct?

server.go Outdated

type handlerListener struct {
s *Server
net.Listener // embedded for Addr

This comment has been minimized.

@iamqizhao

iamqizhao Feb 4, 2016

Contributor

I do not understand the comment. What is "Addr"?

server.go Outdated
type handlerListener struct {
s *Server
net.Listener // embedded for Addr
acceptc chan interface{} // of conn or error

This comment has been minimized.

@iamqizhao

iamqizhao Feb 4, 2016

Contributor

"channel of net.Conn or error"?

@bradfitz bradfitz force-pushed the bradfitz:servehttp branch from eeff984 to e836862 Feb 4, 2016

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

bradfitz commented Feb 4, 2016

Done. PTAL.

server.go Outdated
rawConn, err := hl.Listener.Accept()
if err != nil {
select {
case hl.acceptc <- err:

This comment has been minimized.

@iamqizhao

iamqizhao Feb 5, 2016

Contributor

Since hl.authenticateConn(rawConn) is running in a separate goroutine, this could run into out-of-order issue:
at iteration N: a valid rawConn is generated and h1.authenticateConn is scheduled;
at iteration N+1: an error is returned by hl.Listener.Accept
But line#410 and line#432 could be executed in any order. Assume line#410 in the iteration N+1 gets executed first, then the "conn" in line#432 (iteration N) could be leaked -- the error got read and line#432 is executed (conn is written into hl.acceptc). But nobody will read from hl.acceptc again (http accept loop exits due to the error).

This comment has been minimized.

@iamqizhao

iamqizhao Feb 5, 2016

Contributor

I think handlerListener is complex enough and worth doing some unit tests.

server.go Outdated
}
}

func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {

This comment has been minimized.

@iamqizhao

iamqizhao Feb 5, 2016

Contributor

Comment is required.

@bradfitz bradfitz force-pushed the bradfitz:servehttp branch from e836862 to 1e2a9c2 Feb 5, 2016

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

bradfitz commented Feb 5, 2016

PTAL. I've updated this.

I also got sick of Github's crap pull request code review system, so I'm mirroring this review here:

https://go-review.googlesource.com/#/c/19272/

Feel free to leave comments there instead of here and I'll keep updating both this pull request & that review (increasingly automated)

In particular, Gerrit lets you easily see the diff between versions. Here you can see that I just deleted the net.Listener implementation you didn't like: https://go-review.googlesource.com/#/c/19272/2..3/server.go

@tamird

This comment has been minimized.

Copy link
Contributor

tamird commented Feb 8, 2016

@bradfitz, @iamqizhao: cockroachdb/cockroach#4080 is currently blocked on this. What can I do to help get this over the finish line?

@iamqizhao

This comment has been minimized.

Copy link
Contributor

iamqizhao commented Feb 8, 2016

@tamird

Thanks for early adopting. We will check this in ASAP. It may take a bit more time because it is a big change. Notice that this change directs your traffic onto a new transport which has not been exercised much in grpc world (yes, http2 package is heavily tested and it passes all end2end tests in grpc). So take your own risk here if you want to use it in prod.

@tamird

This comment has been minimized.

Copy link
Contributor

tamird commented Feb 8, 2016

Understood, thanks. Cockroachdb doesn't currently run in "production", so
we're good with taking some risks :)
On Feb 8, 2016 16:04, "Qi Zhao" notifications@github.com wrote:

@tamird https://github.com/tamird

Thanks for early adopting. We will check this in ASAP. It may take a bit
more time because it is a big change. Notice that this change directs your
traffic onto a new transport which has not been exercised much in grpc
world (yes, http2 package is heavily tested and it passes all end2end tests
in grpc). So take your own risk here if you want to use it in prod.


Reply to this email directly or view it on GitHub
#514 (comment).

@bradfitz bradfitz force-pushed the bradfitz:servehttp branch 5 times, most recently from 8c9ca55 to 61d3357 Feb 9, 2016

@LetsUseGerrit

This comment has been minimized.

Copy link

LetsUseGerrit commented Feb 10, 2016

Gerrit code review: https://go-review.googlesource.com/19272 (at git rev 61d3357)

@LetsUseGerrit

This comment has been minimized.

Copy link

LetsUseGerrit commented Feb 11, 2016

Gerrit code review: https://go-review.googlesource.com/19272 (at git rev eb0e517)

@bradfitz bradfitz referenced this pull request Feb 11, 2016

Merged

test: misc cleanups #543

@bradfitz bradfitz force-pushed the bradfitz:servehttp branch from eb0e517 to 81d512c Feb 11, 2016

@LetsUseGerrit

This comment has been minimized.

Copy link

LetsUseGerrit commented Feb 11, 2016

Gerrit code review: https://go-review.googlesource.com/19272 (at git rev 81d512c)

@bradfitz bradfitz force-pushed the bradfitz:servehttp branch from 81d512c to d0f39dd Feb 11, 2016

@LetsUseGerrit

This comment has been minimized.

Copy link

LetsUseGerrit commented Feb 11, 2016

Gerrit code review: https://go-review.googlesource.com/19272 (at git rev d0f39dd)

func (s *Server) useTransportAuthenticator(rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) {
creds, ok := s.opts.creds.(credentials.TransportAuthenticator)
if !ok {
return rawConn, nil, nil

This comment has been minimized.

@zellyn

zellyn Feb 11, 2016

Contributor

Edit: nevermind. Gerrit was just confusing me with its terrible usability. The code does in fact do what I expected.

I'm not sure I understand everything completely, but isn't it likely that the existing server will have already performed the TLS handshake? In that case, we want the GRPC Server not to have creds, and we should try to cast rawConn to a tls.Conn, and return an AuthInfo of TLSInfo{tlsConn.ConnectionState()}

Add a ServeHTTP method to *grpc.Server
This adds new http.Handler-based ServerTransport in the process,
reusing the HTTP/2 server code in x/net/http2 or Go 1.6+.

All end2end tests pass with this new ServerTransport.

Fixes #75

Also:
Updates #495 (lets user fix it with middleware in front)
Updates #468 (x/net/http2 validates)
Updates #147 (possible with x/net/http2)
Updates #104 (x/net/http2 does this)

@bradfitz bradfitz force-pushed the bradfitz:servehttp branch from d0f39dd to 7346c87 Feb 12, 2016

@LetsUseGerrit

This comment has been minimized.

Copy link

LetsUseGerrit commented Feb 12, 2016

Gerrit code review: https://go-review.googlesource.com/19272 (at git rev 7346c87)

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

bradfitz commented Feb 12, 2016

Travis finally scheduled. Build passed.

iamqizhao added a commit that referenced this pull request Feb 12, 2016

Merge pull request #514 from bradfitz/servehttp
Add a ServeHTTP method to *grpc.Server

@iamqizhao iamqizhao merged commit 0631eca into grpc:master Feb 12, 2016

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.01%) to 70.115%
Details
ht.writeCommonHeaders(s)
ht.rw.Write(data)
if !opts.Delay {
ht.rw.(http.Flusher).Flush()

This comment has been minimized.

@tamird

tamird Feb 12, 2016

Contributor

I guess this wasn't in an earlier version of this change, because I'm only noticing the problem now. The presence of this Flush() call makes it unsafe to call Send() on a stream from multiple goroutines. Here's an excerpt from a data race we encounter in our tests https://gist.github.com/tamird/f1bb77e6b5f865c888ed

This comment has been minimized.

@bradfitz

bradfitz Feb 12, 2016

Author Contributor

Thanks! Please file a bug. Clearly we need both a new test, as well as documentation on the ServerTransport interface about the expectations of implementations.

The fix seems easy enough, though.

This comment has been minimized.

@dsymonds

dsymonds Feb 12, 2016

Contributor

I'm not sure a Stream or ClientStream is intended to be safe for concurrent use (except for maybe getting its Context).

This comment has been minimized.

@tamird

tamird Feb 12, 2016

Contributor

Heh, Send() even races with writeFrameAsync. I added mutexes around my own usage and still saw data races https://gist.github.com/91bb4577bced458d01d2

@c4milo

This comment has been minimized.

Copy link
Contributor

c4milo commented Aug 11, 2016

FWIW, while testing https://github.com/philips/grpc-gateway-example as-is, and also testing my own usage of grpc.(*Server).ServeHTTP using Go 1.7rc6, I ran into the issue I just linked above.

@dfawley dfawley referenced this pull request Jul 31, 2017

Merged

Document Server.ServeHTTP #1406

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

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