Document Server.ServeHTTP

[bradfitz]

Fixes #549

[bradfitz]

/cc @dfawley

[puellanivis]

I don’t find this documentation actually any more clear. I think more harnessing of the example might be important. Where is this code snippet supposed to go? How am I supposed to get yourMux? etc.

It still seems like the code is written towards people who already know how the functionality works, and doesn’t need all the “magic” spelled out for them.

[geoffgarside]

The yourMux is just another http.Handler, so whatever would handle your non grpc requests. You might create it from http.NewServeMux() or any of the various http router packages that are available.

You'd probably have this snippet inside some kind of request director ServeHTTP() or http.HandlerFunc middleware.

[puellanivis]

I don’t think putting said explanation here or in a FAQ would be documenting the code clearly.

[dfawley]

Thanks, this is helpful. Could we add a disclaimer to this docstring, as well? Something like:

This functionality is experimental and unsupported.  Serving gRPC
through another HTTP server prevents some gRPC features from
working, and may impact performance significantly.

[dfawley]

I think the docstring here is sufficient to at least point users toward the http package so they can figure out the rest. If more help is necessary, a user manual or examples would be more appropriate IMO. Although, TBH, with the downsides of this method of serving, I would like to discourage its use by all but the biggest power users that already know how this stuff works.

#549 did specifically mention adding usage examples. @bradfitz, do you still have the examples referenced in #549?:

The examples were removed from earlier versions of #514 to reduce the size of that change.

Thanks!

[bradfitz]

I would prefer it as-is without the FUD. The words "may" and "significantly" in your proposed addition are at opposite sides of the certainly spectrum.

If you want to cite actual features they'll lose, that's fine. But which?

[dfawley]

The words "may" and "significantly" in your proposed addition are at opposite sides
of the certainly spectrum.

I don't see these as opposites. "May" describes certainty, while "significantly" describes severity.

If you want to cite actual features they'll lose, that's fine. But which?

Keepalive, keepalive enforcement (ping flood prevention), max connection age and idleness detection, flow control settings and optimizations (e.g. BDP estimation and large message optimizations), stream-level flow control tied to application reads, GracefulStop(), InTapHandle(), stats hooks, status details, and possibly others.

I'd rather keep the FUD levels high here until such time that this path is well-understood and well-supported by the maintainers, which is currently not the case.

[bradfitz]

What is "keepalive"? Go's HTTP/2 server keeps connections alive too, by default.

We also have idle timeouts (https://tip.golang.org/pkg/net/http/#Server ... https://tip.golang.org/pkg/net/http/#Server.IdleTimeout)

We don't do BDP estimation, that's true.

If you want a caveat, I'd prefer a caveat emptor wording that says it's a different HTTP/2 server implementation and "performance and available features may vary", without saying ServeHTTP "may" only suck more. I'd argue in a number of ways that Go's http2 server is more solid.

[bradfitz]

I've pushed an addition with a possible caveat paragraph:

25d5150

[puellanivis]

I like the documentation a lot more now. 👍

[dfawley]

What is "keepalive"? Go's HTTP/2 server keeps connections alive too, by default.

Settings in gRPC to control pings to check liveness (ref), and matching server enforcement rules.

Even if the http2 package supports idle timeouts, we aren't configuring them right now in gRPC. And AIUI, we (within gRPC) can't if we're going through the ServeHTTP method of serving, because we don't have access to the server itself with this mechanism -- unless we hijack the net.Conn away from the server?

ServeHTTP "may" only suck more. I'd argue in a number of ways that Go's http2 server is more solid.

I'm not making any value judgments. But for now, the team is working almost exclusively on the gRPC transport, not the http2 one. We've repeatedly heard that performance is a problem with gRPC through a handler, but we haven't had time to look into this, nor do I expect we will in the next quarter or two. So for now I'd rather lead people (strongly) toward using our server. If our server 1 year from now just fires up x/net/http2, then that would be great, but that is not the current state of things.

I'm OK with the text of the paragraph that is there, but I would like to add a sentence to let people know it is not ("well"?) supported right now.

[dfawley]

@bradfitz asked me to finish this off, as he won't have enough cycles to do so personally. I'll submit it and add the extra sentence I was requesting. Thanks for the PR.