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

Document how to use ServeHTTP #549

Closed
bradfitz opened this Issue Feb 12, 2016 · 34 comments

Comments

Projects
None yet
@bradfitz
Copy link
Contributor

bradfitz commented Feb 12, 2016

Now that #75 is fixed (via #514), let's add examples on how to use ServeHTTP. The examples were removed from earlier versions of #514 to reduce the size of that change.

First, I'd like to get #545 submitted, to clean up the testdata files, before fixing this issue would otherwise make it worse.

/cc @iamqizhao

@ryszard

This comment has been minimized.

Copy link

ryszard commented Mar 1, 2016

Hi, are there any updates here? I really hate having to pass to my binaries both -port and -debug_port, and it's not really trivial to get this to work just from looking at the code in #514...

@xiang90

This comment has been minimized.

Copy link
Contributor

xiang90 commented Mar 1, 2016

@ryszard

While waiting for the "official" doc, probably you can check out
https://github.com/philips/grpc-gateway-example/blob/master/cmd/serve.go.

@iamqizhao

This comment has been minimized.

Copy link
Contributor

iamqizhao commented Mar 1, 2016

We are working on that. But be aware of that is still in the experimental stage and may run into performance regression and stability issues.

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

bradfitz commented Mar 1, 2016

@iamqizhao, can we hold on the FUD until there's data to suggest otherwise? Yes, it's a newer implementation, but the x/net/http2 server is not new at all, and the new ServerTransport implementation isn't much glue between the two.

There might be a bug in the glue, but I don't think it calls for warning about performance problems.

But by all means, if people find problems, please report them.

@iamqizhao

This comment has been minimized.

Copy link
Contributor

iamqizhao commented Mar 1, 2016

@bradfitz

I have not done anything on that yet. But I saw something like https://github.com/cockroachdb/rpc-bench which is not the exact case here but may imply the potential performance regression.

I think it would be good to give a headsup.

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

bradfitz commented Mar 1, 2016

Which numbers on https://github.com/cockroachdb/rpc-bench don't look good?
@tamird

@tamird

This comment has been minimized.

Copy link
Contributor

tamird commented Mar 1, 2016

Huh? rpc-bench doesn't benchmark grpc.(*Server).ServeHTTP at all.

@iamqizhao

This comment has been minimized.

Copy link
Contributor

iamqizhao commented Mar 1, 2016

I meant the when you compare grpc and protoHTTP2.

@iamqizhao

This comment has been minimized.

Copy link
Contributor

iamqizhao commented Mar 1, 2016

for example:

name time/op
GRPC_1K-4 94.7µs ± 4%
ProtoHTTP2_1K-4 167µs ±10%

name speed
GRPC_1K-4 21.6MB/s ± 4%
ProtoHTTP2_1K-4 12.3MB/s ±10%

@tamird

This comment has been minimized.

Copy link
Contributor

tamird commented Mar 1, 2016

Doesn't seem relevant.

@iamqizhao

This comment has been minimized.

Copy link
Contributor

iamqizhao commented Mar 1, 2016

It is relevant because ServeHTTP shares the same transport code as in http2 package.

@tamird

This comment has been minimized.

Copy link
Contributor

tamird commented Mar 1, 2016

Drawing any conclusions about the performance of ServeHTTP from that is not sound. Yes, perhaps the transport is shared, but the remaining 80% of the code is not, since it uses our (cockroachDB's) home-grown protobuf-based RPC codec.

Please do not use this as a reference point.

@iamqizhao

This comment has been minimized.

Copy link
Contributor

iamqizhao commented Mar 1, 2016

On Tue, Mar 1, 2016 at 1:09 PM, Tamir Duberstein notifications@github.com
wrote:

Drawing any conclusions about the performance of ServeHTTP from that is
not sound. Yes, perhaps the transport is shared, but the remaining 80% of
the code is not, since it uses our (cockroachDB's) home-grown
protobuf-based RPC codec.

Don't get me wrong. I have not drawn any conclusions yet. I just mentioned
some possibilities (if you read my message carefully, I mentioned this is
NOT exact apple-to-apple comparison). If you think the difference is from
your custom codec (grpc uses the standard protobuf and protoHTTP2 uses your
own one?), you probably should mention it when you published the results.
Otherwise, it is really misleading when we read the results.

Please do not use this as a reference point.

sure, no problem at all.


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

@zellyn

This comment has been minimized.

Copy link
Contributor

zellyn commented Mar 1, 2016

It doesn't seem confusing: of course it uses a custom protobuf RPC codec: there is no public protobuf-based RPC mechanism. That is what GRPC will be.

@iamqizhao

This comment has been minimized.

Copy link
Contributor

iamqizhao commented Mar 1, 2016

It seems at least I am getting confused here. :-) For the published results, what codec are used for
i) GRPC
ii) ProtoHTTP2
respectively. Are they same?

@tamird

This comment has been minimized.

Copy link
Contributor

tamird commented Mar 1, 2016

No, they are not the same. ProtoHTTP2 is using http://godoc.org/github.com/cockroachdb/cockroach/rpc/codec.

@iamqizhao

This comment has been minimized.

Copy link
Contributor

iamqizhao commented Mar 1, 2016

Thanks, tamird.

@zellyn, this is the reason why it is confusing -- GRPC and ProtoHTTP2 used two different codecs in benchmarks...

@tamird

This comment has been minimized.

Copy link
Contributor

tamird commented Mar 2, 2016

I've updated https://github.com/cockroachdb/rpc-bench to include grpc.(*Server).ServeHTTP and indeed the numbers look pretty bad compared to grpc.(*Server).Serve. See cockroachdb/rpc-bench#9

@ryszard

This comment has been minimized.

Copy link

ryszard commented Mar 2, 2016

@xiang90 Thanks a lot for the link! However, it doesn't really work for me, and it's not obvious to me if I am doing something wrong, or there's something about using grpc-gateway, swagger, etc. that makes it work in the example.

My code looks kinda like this:

// grpcHandlerFunc returns an http.Handler that delegates to
// grpcServer on incoming gRPC connections or otherHandler
// otherwise. Copied from cockroachdb.
func grpcHandlerFunc(rpcServer *rpc.Server, otherHandler http.Handler) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        // TODO(tamird): point to merged gRPC code rather than a PR.
        log.Info("here")
        log.Infof("r: %#v r.Header: %v", r, r.Header)
        if r.ProtoMajor == 2 && strings.Contains(r.Header.Get("Content-Type"), "application/grpc") {
            log.Infof("in grpc")
            rpcServer.ServeHTTP(w, r)
        } else {
            log.Infof("in normal server")
            otherHandler.ServeHTTP(w, r)
        }
    })
}

func main() {
    // other, unrelated stuff

    server := rpc.NewServer(opts...)
    dm := NewMyServer()
    pb.RegisterCapacityServer(server, dm)
    srv := &http.Server{
        Addr:    fmt.Sprintf(":%d", *port),
        Handler: grpcHandlerFunc(server, http.DefaultServeMux),
    }

    log.Exit(srv.Serve(lis))
}

The HTTP part works fine, but as soon as I try to send an RPC using a Go client, then it goes in the wrong branch. The request looks like:

&http.Request{Method:"PRI", URL:(*url.URL)(0xc82043e780), Proto:"HTTP/2.0", ProtoMajor:2, ProtoMinor:0, Header:http.Header{}, Body:(*struct { http.eofReaderWithWriteTo; io.Closer })(0xc97990), 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:"[::1]:57688", RequestURI:"*", TLS:(*tls.ConnectionState)(nil), Cancel:(<-chan struct {})(nil)} r.Header: map[]

So, it goes down the wrong path because the content-type is not set the to application/grpc.

All this both in Go 1.5 and 1.6.

@bradfitz Can you please shed some light on this?

Oh, and this is for experimental code. I am fine with performance regressions, bugs, etc.

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

bradfitz commented Mar 2, 2016

The http.Handler-based server only works with TLS right now.

The "PRI" you're seeing is the dummy kinda-not-really-HTTP request that is announced by http2 clients during upgrade.

If you used TLS, though, the x/net/http2 package (or Go 1.6's default server) would do the upgrade for you. But neither x/net/http2 or Go itself automatically upgrades unencrypted connections to http2, so you're seeing the PRI request.

I plan to make this work for gRPC, but it's not a super high priority at the moment.

@c4milo

This comment has been minimized.

Copy link
Contributor

c4milo commented Aug 12, 2016

The http.Handler-based server only works with TLS right now.

@bradfitz I haven't been able to make it work, even with TLS. This is the error I get philips/grpc-gateway-example#11

@BrianHicks

This comment has been minimized.

Copy link

BrianHicks commented Aug 26, 2016

I'm seeing the same behavior in Go 1.7.

@gdm85

This comment has been minimized.

Copy link

gdm85 commented Aug 27, 2016

(Tried with 1.5 and 1.7 here)
I was trying to arrange a PR for this issue, adding an example similar to @ryszard's code and based on the greeter example, but I get:

2016/08/27 13:31:00 transport: http2Client.notifyError got notified that the client transport was broken write tcp 127.0.0.1:45990->127.0.0.1:50051: write: connection reset by peer.
2016/08/27 13:31:00 could not greet: rpc error: code = 13 desc = transport: write tcp 127.0.0.1:45990->127.0.0.1:50051: write: connection reset by peer

So I don't think ServeHTTP is usable at the moment (I tested only without TLS), the best pick would be @soheilhy's cmux for the time being.

I guess next step would be to add proper servehttp_test.go tests to cover the malfunctions?

For anyone interested, the server-side is here: 79b7c34...gdm85:master

The client-side greeter example can be used to test this.

My end goal would be to have HTTP 1.x health checks support (e.g. /status replying with a 200 status code in case of healthy service and 5xx otherwise) and eventually human-readable HTML, which I achieved using cmux in this POC: https://github.com/gdm85/grpc-go-multiplex

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

bradfitz commented Aug 27, 2016

Can you run the server with Go 1.7 and env GODEBUG=http2debug=2 and report what it says when grpcServer.ServeHTTP fails?

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

bradfitz commented Aug 27, 2016

I guess next step would be to add proper servehttp_test.go tests to cover the malfunctions?

It did have tests. I'm curious what could've regressed.

@gdm85

This comment has been minimized.

Copy link

gdm85 commented Aug 27, 2016

Can you run the server with Go 1.7 and env GODEBUG=http2debug=2 and report what it says when grpcServer.ServeHTTP fails?

The client-side shows:

2016/08/27 23:31:57 http2: Framer 0xc420196180: wrote SETTINGS len=0
2016/08/27 23:31:57 http2: Framer 0xc420196180: wrote WINDOW_UPDATE len=4 (conn) incr=983025
2016/08/27 23:31:57 grpc: addrConn.resetTransport failed to create client transport: connection error: desc = "transport: write tcp 127.0.0.1:45945->127.0.0.1:50051: write: broken pipe"; Reconnecting to {"localhost:50051" <nil>}
2016/08/27 23:31:57 could not greet: rpc error: code = 14 desc = grpc: the connection is unavailable
exit status 1

However, if you look at the server I'm using (79b7c34...gdm85:master), I might say that this is not enabling HTTP2 at all before serving gRPC. I have already tried adding http2.ConfigureServer(srv, &http2.Server{}), same result (thus this line is not there right now). This is reinforced by the fact no extra debug output is generated by this server (thus http2 is probably not used), while I can see it with the vanilla greeter server.

@c4milo

This comment has been minimized.

Copy link
Contributor

c4milo commented Aug 27, 2016

The only way I was able to make it work was using explicitly srv.ListenAndServeTLS

@gdm85

This comment has been minimized.

Copy link

gdm85 commented Aug 27, 2016

It did have tests. I'm curious what could've regressed.

@bradfitz I had looked at your PR (#514) before commenting here, I understand it's a refactoring step, but I see no explicit test for .Serve() as used by me/others (or .ListenAndServe(), would be equivalent) - maybe that's the missing test?

Sorry if my mention of tests was confusing, I didn't mean any regression happened but just that the bug/missing feature could be added by first writing the (failing) test for it (as a work-in-progress PR), then adjusting the code until test passes.

@gdm85

This comment has been minimized.

Copy link

gdm85 commented Aug 27, 2016

@c4milo can you also check the example I mentioned above (basically @ryszard's adjusted code)? Does it seem correct? @bradfitz had mentioned he needs to make this work, so I think nothing new here to report.

@atombender

This comment has been minimized.

Copy link

atombender commented Oct 15, 2016

The http.Handler-based server only works with TLS right now.

What's needed to support non-TLS?

@devoid

This comment has been minimized.

Copy link

devoid commented Mar 3, 2017

Ok digging through issue #75 and this one, I found that @ryszard's example in #549 (comment) worked for a TLS enabled http.ServeMux.

Can we at least document this approach if it is workable for users of TLS?

@Raffo

This comment has been minimized.

Copy link

Raffo commented Jul 25, 2017

@atombender Do you have an idea if this got fixed or if there is any plan to fix that? It's really annoying and I would need HTTP for production as well, I don't need termination in the APP (I am using a proxy).

@atombender

This comment has been minimized.

Copy link

atombender commented Jul 25, 2017

@Raffo No idea.

bradfitz added a commit to bradfitz/grpc-go that referenced this issue Jul 31, 2017

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

bradfitz commented Jul 31, 2017

I sent #1406 with some docs.

bradfitz added a commit to bradfitz/grpc-go that referenced this issue Jul 31, 2017

@dfawley dfawley closed this in #1406 Aug 7, 2017

dfawley added a commit that referenced this issue Aug 7, 2017

@lock lock bot locked as resolved and limited conversation to collaborators Sep 26, 2018

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