Skip to content
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

Allow enabling HTTP/2 PING and disabling read timeout for streams #13

Closed
kohenkatz opened this issue Mar 5, 2023 · 2 comments · Fixed by #128
Closed

Allow enabling HTTP/2 PING and disabling read timeout for streams #13

kohenkatz opened this issue Mar 5, 2023 · 2 comments · Fixed by #128

Comments

@kohenkatz
Copy link
Contributor

The default okhttp readTimeout is 10 seconds, which is too short for many uses of streaming RPCs. As an alternative way to detect broken connections, HTTP/2 provides a PING mechanism. okhttp has pings disabled by default, but they can be enabled by calling pingInterval() on the builder. Streaming calls can set readTimeout to 0 to disable the read timeout and rely only on the ping timeout, if desired.

The problem is that there is not an easy way to change the okhttp settings for unary vs. streaming calls, and we still want to have a read timeout for the unary calls.

I thought it would be easiest to fix this by extending ConnectOkHttpClient to customize the client object and then call the parent method using the customized client, but ConnectOkHttpClient would need to be open instead of final to allow this.

I ended up doing this, which feels like a nasty hack:

class PingingConnectClient(client: OkHttpClient): HTTPClientInterface {
    private val internalUnaryClient = ConnectOkHttpClient(client)
    private val internalStreamClient = ConnectOkHttpClient(client.newBuilder()
        .pingInterval(30, TimeUnit.SECONDS)
        .readTimeout(0, TimeUnit.SECONDS)
        .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)
    }
}
@pkwarren
Copy link
Contributor

pkwarren commented Oct 9, 2023

@kohenkatz - This seems like a reasonable request. Would it work for you to just change the constructor to:

class ConnectOkHttpClient @JvmOverloads constructor(
    private val unaryClient: OkHttpClient = OkHttpClient(),
    private val streamClient: OkHttpClient = unaryClient,
) : HTTPClientInterface {

Then in stream, we'd just update to use streamClient.

@kohenkatz
Copy link
Contributor Author

kohenkatz commented Oct 9, 2023

@pkwarren Yes, that sounds like it would do what I want.

pkwarren added a commit that referenced this issue Oct 10, 2023
Streaming calls often require longer timeouts (for long lived streams)
and enabling pings (to keep the underlying connection alive). Update
ConnectOkHttpClient to take a second OkHttpClient argument to the
constructor to be used for streaming calls.

Fixes #13.
pkwarren added a commit that referenced this issue Oct 11, 2023
Streaming calls often require longer timeouts (for long lived streams)
and enabling pings (to keep the underlying connection alive). Update
ConnectOkHttpClient to take a second OkHttpClient argument to the
constructor to be used for streaming calls.

Fixes #13.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants