kotlinx.coroutines icon indicating copy to clipboard operation
kotlinx.coroutines copied to clipboard

Guidance on custom implementations of coroutine dispatchers, and usage of `Delay`

Open martaciesielska opened this issue 2 years ago • 6 comments

I've been working on some code that uses a custom implementation of CoroutineDispatcher for testing. Our test dispatcher uses fake time, and runs everything on a single thread to ensure that tests are deterministic. It's not appropriate for end-to-end testing but it works well with a lot of unit tests (simplicity, speed, determinism). Due to the inner workings of Kotlin coroutines, we made our custom dispatcher implement the Delay interface to be able to control scheduling, even though Delay is marked as an internal API.

Additionally to the test dispatcher, we want to maintain a coroutine dispatcher decorator that facilitates dispatch/suspension observability. This decorator would look something like this:

internal class CoroutineDispatcherDecorator(
  private val delegate: CoroutineDispatcher,
  private val observer: Observer, // our interface for observing some events in the system
) : CoroutineDispatcher() {

  override fun dispatch(context: CoroutineContext, block: Runnable) {
    observer.onDispatch(context)
    delegate.dispatch(context) {
      try {
        block.run()
      } finally {
        observer.onSuspend(context)
      }
    }
  }
}

With this decorator, we run into another problem with Delay: if delegate actually implements it, we hide it by using the decorator.

Given the above, I wanted to ask some questions:

  1. Are there any plans to make Delay public or provide some other public API in its place? I've seen one issue from 2020 where it was actually recommended to someone to implement Delay, is this advice up-to-date? I understand that if we use Delay, we run the risk of our code getting broken with some future release of Kotlin coroutines.

  2. If not, are custom implementations of dispatchers discouraged? Neither CoroutineDispatcher nor ContinuationInterceptor are marked as internal so at first glance it seems like this is OK but not implementing Delay could lead to unexpected behaviour. Is there any guidance on this (I haven't found it in the documentation but maybe I've missed it)?

martaciesielska avatar May 23 '23 13:05 martaciesielska

Why do you want to implement your custom CoroutineDispatcher for testing with fake time, when we already provide such a mechanism https://github.com/Kotlin/kotlinx.coroutines/tree/master/kotlinx-coroutines-test?

I think you raise an interesting point regarding cases when you want to decorate a dispatcher while keeping the Delay implementation. This is not my first time seeing people wrapping our dispatchers with something. Maybe we should provide some proper way to do just that while keeping the Delay implementation, something like class CoroutineDispatcherWrapper(wrapped: CoroutineDispatcher): CoroutineDispatcher(), Delay. Would this cover your use case?

dkhalanskyjb avatar May 23 '23 13:05 dkhalanskyjb

Why do you want to implement your custom CoroutineDispatcher for testing with fake time, when we already provide such a mechanism https://github.com/Kotlin/kotlinx.coroutines/tree/master/kotlinx-coroutines-test?

  1. It's marked as experimental which is a bit of a deterrent (maybe not as strong as internal but still).

  2. It doesn't give us the same amount of flexibility - apart from manipulating time, in some cases we also enforce certain rules as to which work items get priority for execution, to simulate race conditions that we know might occur in a live system.

  3. On the flip side of point 2), having our own dispatcher also gives us an opportunity to remove complexity where we don't need it if we're not using particular coroutines' features, which can make for very easy debugging.

I think you raise an interesting point regarding cases when you want to decorate a dispatcher while keeping the Delay implementation. This is not my first time seeing people wrapping our dispatchers with something. Maybe we should provide some proper way to do just that while keeping the Delay implementation, something like class CoroutineDispatcherWrapper(wrapped: CoroutineDispatcher): CoroutineDispatcher(), Delay. Would this cover your use case?

That would definitely help with the decorator we want to put in our production code.

I'd still be keen to have a mechanism to implement my own dispatcher with scheduling support for testing, for the reasons listed above. Albeit not ideal, I'd be fine with using the wrapper if it allowed intercepting scheduling.

martaciesielska avatar May 24 '23 11:05 martaciesielska

Hi! Any updates on this? I’d really be keen to stop relying on the internal API for the decorator at least.

martaciesielska avatar Aug 02 '23 09:08 martaciesielska

Not many updates, unfortunately. For Delay to be useful, it requires a non-trivial rework and rethinking from the ground up, and it's slightly out of our scope right now.

Depending on internal API is okay-ish as long as it's done on the application level, not on the library one (e.g. Delay is not used by 3rd-party libraries that are shipped to be used by other clients that also depend on coroutines), Delay won't be broken on a single release basis and there will be a proper deprecation cycle

qwwdfsad avatar Aug 04 '23 14:08 qwwdfsad

Is it ok to wrap CoroutineDispatcher like this:

abstract class CoroutineDispatcherWrapper(
    private val dispatcher: CoroutineDispatcher,
) : CoroutineContext.Element {

    override val key: CoroutineContext.Key<*> = dispatcher.key

    override fun <R> fold(initial: R, operation: (R, CoroutineContext.Element) -> R): R {
        return operation(initial, dispatcher)
    }

    override fun <E : CoroutineContext.Element> get(key: CoroutineContext.Key<E>): E? {
        @Suppress("UNCHECKED_CAST")
        return if (this.key == key) dispatcher as E else null
    }

    override fun minusKey(key: CoroutineContext.Key<*>): CoroutineContext {
        return if (this.key == key) EmptyCoroutineContext else dispatcher
    }
}

looks like in this case wrapper is disappearing when added to CoroutineScope and this is exactly what I need. If you curios why am I doing this, it's for DI. In my team we are always using concrete classes when requesting dependencies. For example with RxJava:

class SomeRepository(
    private val workerScheduler: WorkerScheduler, 
    private val timerScheduler: TimerScheduler,
) {
    // ...
}

where WorkerScheduler and TimerScheduler are just our named wrappers around io.reactivex.Scheduler. I'm trying to do same thing with CoroutineDispatcher's and now I'm curios is it ok to wrap them like this or it can cause some unexpected problems?

bejibx avatar Aug 10 '23 06:08 bejibx

Maybe we should provide some proper way to do just that while keeping the Delay implementation, something like class CoroutineDispatcherWrapper(wrapped: CoroutineDispatcher): CoroutineDispatcher(), Delay.

FWIW, I think this would be helpful. Our use case is somewhat similar...we want to add Espresso idling resource wrappers for the coroutine dispatchers.

kenyee avatar Jun 24 '24 14:06 kenyee