Add timeout enforcement to ProtocolClient #276
Conversation
| // | ||
| // The default oracle, if not configured, returns a 10 second timeout for | ||
| // all operations. | ||
| val timeoutOracle: TimeoutOracle = { 10.toDuration(DurationUnit.SECONDS) }, |
There was a problem hiding this comment.
I added this to account for removal of the timeouts in the configuration helper below. So if users use ConnectOkHttpClient.configureClient and forget to set timeouts in this config, they get the same default behavior as the OkHttpClient was providing (except that this default applies to bidirectional streams, whereas the OkHttpClient timeouts do not).
|
@pkwarren, ping. |
| val DEFAULT_SCHEDULER = object : Scheduler { | ||
| override fun scheduleTimeout(delay: Duration, action: Cancelable): Timeout { | ||
| val timeout = Timeout(action) | ||
| val task = timerTask { timeout.trigger() } |
There was a problem hiding this comment.
This works but we might consider tradeoffs vs. ScheduledThreadPoolExecutor. Probably not a big deal for timeout scheduling.
There was a problem hiding this comment.
I can change it. When I was searching, trying to figure out the idiomatic way to do this in Kotlin and Android apps, this was cited more than use of a ScheduledExecutorService. There were also a couple of Android-specific ways to do it, and then there was the coroutine way (that I couldn't get to work correctly -- to just create a coroutine and then delay(...) in the coroutine before executing the action).
For this, there's not really much of a tradeoff -- both solutions are roughly equivalent. Both approaches require creation of a heavyweight thread. Both implementations are similar, using a thread that polls a priority queue and then executes the tasks. The only potentially meaningful difference is that ScheduledExecutorService impls use a DelayQueue and the stuff in java.util.concurrent whereas the Timer just uses intrinsic locks and Object.wait and Object.notify.
But it's super easy to switch if you think that's more appropriate.
| unaryClient: OkHttpClient = OkHttpClient(), | ||
| streamClient: OkHttpClient = unaryClient, | ||
| private val unaryClient: OkHttpClient = OkHttpClient(), | ||
| // TODO: remove this; a separate client is only useful for configuring separate timeouts, |
There was a problem hiding this comment.
This is not just for timeouts IIRC - you may also want to configure pings on clients for long lived streams but not on ones for unary calls.
There was a problem hiding this comment.
Here's some background on the two clients (aside from timeouts which will be much better behaved with this PR): #13
There was a problem hiding this comment.
It's unclear why you'd want PINGs on one client and not the other. If you have created a connection that uses PINGs, just use that same client for both unary and stream traffic. I don't see any downside to that. It wouldn't increase the PING activity since you're not creating two clients that both use PING -- but instead using the PINGing client for all RPCs.
As far as the read timeout being different between unary and streams, that is now even more configurable (based on MethodSpec) via the timeout oracle. So you can set the read timeout to zero (or use ConnectOkHttpClient.configureClient) and it should be appropriate for both unary and stream traffic.
@kohenkatz, please take a look at this thread and PR and let me know what you think. Note that this PR is not removing the second client -- not yet. But I don't think it will be needed anymore and would appreciate your input, like if you see other reasons to keep the second client in future versions.
This moves the timeout enforcement out of the HTTPClientInterface and into the ProtocolClientInterface. This is more appropriate so that the protocol implementations can be aware of the timeout and add a timeout header, to propagate the deadline to the server.
I've broken it up into a sequence of commits that hopefully makes it easier to review.
I tried looking into making the timeout scheduler coroutine-aware. But this caused issues. Even though I could propagate the
CoroutineScopefrom the caller, when invoked from a unary suspend function or a stream function, the issue is howCoroutineScope.launchimpacts the scope lifetime. In particular, a scope can't complete until all of its children complete, so I was running into issues where code was incorrectly blocking on that child coroutine (which was just doingdelayfor the full timeout period) to end.I didn't debug it enough to get to bedrock regarding where the blocking was happening or exactly why (naively, I would have expected the timeout coroutine to just be a part of the main coroutine scope create in the initial
runBlockingin the conformance client, which shouldn't cause the blocking issues I observed 🤷).So using a simpler Timer seemed like the next best thing. And an alternative like this was needed anyway to be called from non-suspend functions like the callback and blocking unary signatures (for which there is no parent coroutine scope in which to launch a new coroutine).
Resolves #249.