Force HTTP/2 (h2c) for streams when URL is `http://...`

[kohenkatz]

The gRPC libraries for Java/Kotlin (and most other languages) provide a usePlaintext() builder method that tells the library to communicate using HTTP/2 over Cleartext (h2c). This is mostly useful for communicating with development servers running on the developers' PC and for certain types of automated testing.

Because connect-kotlin uses a full URL instead of separate host and port arguments, we can check whether the scheme is http or https to determine whether to enable plaintext support.

We for want to keep HTTP/1.1 support for unary connect calls, which is similar to the discussion in #13 about separate client options for unary vs. streaming. However, gRPC requires HTTP/2 for unary calls too, so we probably need to check the chosen protocol before setting this option.

Here's what I'm using for this right now (built on top of my code sample in #13). Note that this cannot use automatic detection based on the URL scheme and network protocol because the scheme is not available when the client is created. Instead, the constructor has two additional arguments:

class PingingConnectClient(client: OkHttpClient, usePlaintext: Boolean, networkProtocol: NetworkProtocol): HTTPClientInterface {
    private val internalUnaryClient = ConnectOkHttpClient(client.newBuilder()
        .apply {
            // Unary gRPC uses HTTP/2, but connect can still use HTTP/1.1
            if (networkProtocol == NetworkProtocol.GRPC && usePlaintext) {
                protocols(listOf(Protocol.H2_PRIOR_KNOWLEDGE))
            }
        }
        .build())

    private val internalStreamClient = ConnectOkHttpClient(client.newBuilder()
        .pingInterval(30, TimeUnit.SECONDS)
        .readTimeout(0, TimeUnit.SECONDS)
        .apply {
            // Streaming always must use HTTP/2
            if (usePlaintext) {
                protocols(listOf(Protocol.H2_PRIOR_KNOWLEDGE))
            }
        }
        .build())

    override fun unary(request: HTTPRequest, onResult: (HTTPResponse) -> Unit): Cancelable {
        return internalUnaryClient.unary(request, onResult)
    }

    override fun stream(
        request: HTTPRequest,
        onResult: suspend (StreamResult<Buffer>) -> Unit
    ): Stream {
        return internalStreamClient.stream(request, onResult)
    }
}

I suspect that it makes the most sense to implement this in ConnectOkHttpClient as follows (not tested), but I'm not totally sure:

class ConnectOkHttpClient(
    val client: OkHttpClient = OkHttpClient()
) : HTTPClientInterface {
    override fun unary(request: HTTPRequest, onResult: (HTTPResponse) -> Unit): Cancelable {
        val unaryClient = client.newBuilder().apply {
            if (request.url.protocol == "http" && request.contentType.startsWith("application/grpc")) {
                protocols(listOf(Protocol.H2_PRIOR_KNOWLEDGE))
            }
        }.build()
        // The rest of the existing function here, but replace `client.newCall` with `unaryClient.newCall`
    }

    override fun stream(request: HTTPRequest, onResult: suspend (StreamResult<Buffer>) -> Unit): Stream {
        val streamClient = client.newBuilder().apply {
            if (request.url.protocol == "http") {
                protocols(listOf(Protocol.H2_PRIOR_KNOWLEDGE))
            }
        }.build()
        return client.initializeStream(request, onResult)
    }

[jhump]

The first example (injecting this logic into generated code) would tightly couple the generated client stubs to the existing okhttp implementation. But the purpose of this being an interface (HTTPClientInterface) is to allow other possible implementations and avoid such tight coupling.

The latter example (pushing the logic into the HTTPClientInterface implementation) makes much more sense to me. However, it would need to be more clever than the example since the example builds a new client for each RPC, which will be inefficient since it prevents client re-use and connection pooling and would require more logic around connection/client clean-up when each RPC completes.

So a little more exploration is needed to come up with how we might apply an idea similar to the latter example, but still able to re-use clients and pool connections. We also need to make sure the solution doesn't cause resource leaks: okhttp clients are heavyweight and stateful (using both network connections as well as worker threads in a thread pool) and need to be closed/freed when no longer needed. If the solution involves the ConnectOkHttpClient needing to create one or more clients, it must also expose a way to close them and safely shut them down.

[kohenkatz]

@jhump The first example is my interim workaround - I did not expect that it would be the correct way to do it long term. Given that the code has changed quite a bit since I first raised this issue, I will see if I can take a fresh look at it to try to suggest another way to do this.

[jhump]

@kohenkatz, I believe this is still an issue. The HTTPClientInterface and ConnectOkHttpClient types have not changed that much.

BTW, one downside to having this be automatic/implicit (i.e. based on detecting the gRPC protocol, as in the example) is that it means different configuration would be needed in order to use H2C with gRPC-Web and Connect protocol traffic. That inconsistency (that sometimes H2C is used when not explicitly requested, but not always) could be a source of confusion/surprise.