Executor.asCoroutineDispatcher() may not use its input for Delay

[yorickhenning]

Executor.asCoroutineDispatcher() does a couple troubling things:

1. It may silently fall back to DefaultExecutor for scheduling delays - even if its input inherits from ScheduledExecutorService
2. It calls setRemoveOnCancelPolicy() on its input - a method with side-effects - and doesn't document doing so

The Executor type hierarchy is messy, but ScheduledExecutorService is the type that adds support for delaying function execution. My reading of asCoroutineDispatcher() was that it would use the threads belonging to its input ScheduledExecutorService to implement the Delay functions.

As implemented though, asCoroutineDispatcher() doesn't detect the capabilities of its input. If the runtime type of the Executor passed to asCoroutineDispatcher() isn't exactly ScheduledThreadPoolExecutor - which isn't the only way to implement a pool-backed ScheduledExecutorService - asCoroutineDispatcher() will ignore that its input can be used to run delayed continuations. It'll silently fall back to a statically allocated thread pool and run delayed continuations on those threads instead.

We experienced bugs because the statically allocated Threads weren't configured with necessary ThreadLocal state and monitoring. Both calls to delay() and reading the ThreadLocals are individually rare enough that the creation of new threads escaped notice in automated testing.

I propose the following fixes:

1. Document the side-effecting call to setRemoveOnCancelPolicy() on the function, why it was added, and that it's subject to change
2. Remove or make optional the reflective call to setRemoveOnCancelPolicy()
3. Fix encapsulation by modifying asCoroutineDispatcher() to implement Delay using its input, if the input is any subtype of ScheduledExecutorService

Something like this:

fun Executor.asCoroutineDispatcher(): CoroutineDispatcher {
  return if (this is ScheduledThreadPoolExecutor) {
    // Implement `Delay` using `this`. Maybe run `setRemoveOnCancelPolicy()` if documented/flagged.
  } else if (this is ScheduledExecutorService) {
    // Implement `Delay` using `this`.
  } else if (this is ExecutorService) {
    // Implement (and document) that `Delay` will be implemented by using a statically allocated
    // DefaultExecutor.
  } else if (this is DispatcherExecutor) {
    this.dispatcher
  } else {
    // Implement (and document) that `Delay` will be implemented by using a statically allocated
    // DefaultExecutor.
  }

// Or, don't avoid that extensions are resolved statically and write overloads.
// I think this prevents the DispatcherExecutor.asCoroutineDispatcher().asExecutor()
// optimization.
fun ScheduledThreadPoolExecutor.asCoroutineDispatcher(callSetRemoveOnCancelPolicy = true) {}
fun ScheduledExecutorService.asCoroutineDispatcher() {}
fun Executor.asCoroutineDispatcher() {}

I’m happy to draft some patches to get this working more effectively.

[qwwdfsad]

Thanks for the detailed write-up!
Your proposal indeed looks great and worth adding to 1.5. PR is not necessary, but very welcome.
Regarding setRemoveOnCancelPolicy, I'm not yet sure whether we actually want to make that optional (or defaults to false). It was added intentionally (and is called reflectively mostly for the sake of Java 1.6 compatibility) in order to avoid memory-leak in the following (boiled down) scenario:

while (true) {
    launch(executorDispatcher) {
         delay(hugeDelay)
    }.cancel()
}

Cancellation is much more common in coroutines than in Java's delay tasks, so getting rid of setRemoveOnCancelPolicy may cause unexpected memory pressure and/or potential OOMs. But it definitely worth documenting and explaining the rationale in the documentation