okhttp icon indicating copy to clipboard operation
okhttp copied to clipboard

Provide a clean way for Bridging Interceptors to get a cancel signal

Open yschimke opened this issue 2 years ago • 8 comments

Application Interceptor that use an alternate transport and short circuit the interceptor chain don't have a clean way to handle cancellation.

They can either poll the call, or possibly set a specific EventListener and handle the cancel event to get this signal.

yschimke avatar Mar 17 '22 15:03 yschimke

We might also want to give some way to access the EventListener (internal val on RealCall), if we'd like that to be part of the contract for these bridging interceptors.

yschimke avatar Mar 17 '22 17:03 yschimke

Also wondering when implementing an Interceptor in this new world. Is this a useful base class?

abstract class CoroutineInterceptor() : Interceptor {
  final override fun intercept(chain: Interceptor.Chain): Response {
    return runBlocking() {
      try {
        interceptSuspend(chain)
      } catch (ce: CancellationException) {
        throw IOException(ce)
      }
    }
  }

  abstract suspend fun interceptSuspend(chain: Interceptor.Chain): Response
}

And should it cancel the runBlocking call automatically when the call is cancelled?

Not an issue if you are blocking on the call or response, but if you are doing other work around the interceptor, including another forked request (auth) then the cancellation would be helpful.

yschimke avatar Apr 10 '22 12:04 yschimke

I suspect for bridging interceptors currently they end up looking like

  override fun intercept(chain: Interceptor.Chain): Response {
    return runBlocking {
      makeRequest(engine, chain.request())
    }
  }
}

suspend fun makeRequest(engine: Engine, request: Request) =
  suspendCancellableCoroutine<Response> { continuation ->
...
        .setOnResponseTrailers { responseTrailers, streamIntel ->
          if (chain.call().isCanceled()) {
            continuation.cancel(CancellationException("underlying connection was cancelled"))
            return@setOnResponseTrailers
          }

yschimke avatar Apr 10 '22 12:04 yschimke

I'm out of my depth writing a test for this, hence why it would be useful to have a solid library implementation to simplify these cases.

yschimke avatar Apr 10 '22 13:04 yschimke

Definitely runBlocking is wrong here. If you want to use coroutines in the call to proceed() I think you're in for a bad time.

swankjesse avatar Apr 11 '22 04:04 swankjesse

Updated mine to runBlocking without IO. Not sure why runBlocking is wrong here, it achieves the same thing as the standard blocking call, uses the same thread, but allows you to do asynchronous operations elegently.

runBlocking:

* The default [CoroutineDispatcher] for this builder is an internal implementation of event loop that processes continuations
* in this blocked thread until the completion of this coroutine.
* See [CoroutineDispatcher] for the other implementations that are provided by `kotlinx.coroutines`.

yschimke avatar Apr 11 '22 07:04 yschimke

This post gets into it. https://www.billjings.com/posts/title/foot-marksmanship-with-runblocking/

Coroutines are cooperative multitasking. Running a blocking call or runBlocking inside a coroutine is uncooperative and bad non-local things can happen.

swankjesse avatar Apr 11 '22 11:04 swankjesse

I don't agree with all of that

There's another reason it won't cancel besides breaking the job chain, though, which is that runBlocking behaves just like Thread.sleep. That is, when you call it it will hard block the thread — the thread will sit there and wait for runBlocking to return.

According to docs, the thread doesn't sleep, it's a single threaded executor for the coroutines. So if the work remains non blocking, it shouldn't deadlock, and it shouldn't magically start using a bunch of extra threads. This sounds like what you want when you have a synchronous interface like Interceptor.

For your comment:

Running a blocking call or runBlocking inside a coroutine is uncooperative and bad non-local things can happen.

Agreed, but that's not what I'm proposing. A blocking call to intercept that has nothing to do with the thread until it finishes, can donate that thread to run the coroutines.

Uuugggghhhhh... I guess it breaks if the intercept method makes calls using enqueue, because they may not be processed immediately, so will waiting (noon-blockingly) for a request that can't complete until the call triggering intercept completes.

yschimke avatar Apr 11 '22 12:04 yschimke