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

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

Open
yschimke opened this issue Mar 17, 2022 · 8 comments · May be fixed by #7447
Open

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

yschimke opened this issue Mar 17, 2022 · 8 comments · May be fixed by #7447
Labels
enhancement Feature not a bug
Milestone

Comments

@yschimke
Copy link
Collaborator

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 yschimke added the enhancement Feature not a bug label Mar 17, 2022
@yschimke
Copy link
Collaborator Author

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
Copy link
Collaborator Author

yschimke commented Apr 10, 2022

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
Copy link
Collaborator Author

yschimke commented Apr 10, 2022

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
Copy link
Collaborator Author

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.

@swankjesse
Copy link
Member

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.

@yschimke
Copy link
Collaborator Author

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`.

@swankjesse
Copy link
Member

swankjesse commented Apr 11, 2022

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.

@yschimke
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature not a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@swankjesse @yschimke and others