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

DefaultDispatcher #136

Closed
elizarov opened this issue Sep 28, 2017 · 11 comments
Closed

DefaultDispatcher #136

elizarov opened this issue Sep 28, 2017 · 11 comments

Comments

@elizarov
Copy link
Contributor

This is write-up on why kotlinx.coroutines needs a DefaultDispatcher and what it is.

The plan is to have a default value of context parameter for all coroutine builders that are defined in kotlinx.coroutines, so that instead of launch(someContext) { ... } we can just write launch { ... } unless we really care what context our coroutine runs in. Moreover, sometimes we only care about non-dispatch aspects of our coroutines and write launch(CoroutineName("foo")) { ... } which does not really work as of now (see #133). We need some clearly defined default dispatcher that would be used in this case.

This is especially important in light of our effort to bring kotlinx.coroutines into Kotlin/JS and Kotlin/Native code in such a way as writing common code with coroutines becomes possible. In common code we cannot write launch (CommonPool) { ... } since the CommonPool is defined only in Kotlin/JVM and is meaningless on other platforms due to different threading model. However, each platform can provide its own sensible default dispatcher.

We are going to define a top-level val in kotlinx.coroutines.experimental package:

val DefaultDispatcher: CoroutineDispatcher = ...

There are a few questions to be answered here :

  1. The type. We can use both CoroutineContext and CoroutineDispatcher as types of that variable. The latter is preferable, since it would clearly highlight that it provides default dispatching logic and would prevent some obvious mistakes like DefaultDispatcher + UI (it will get flagged as an error during compilation, because adding two dispatchers does not make sense, as the later overwrites the former).

  2. The name. DefaultDispatcher looks good, since it clearly explains the purpose and fits the type. TheCaptilazationOfTheName is choosen for consistency with other dispatcher names like UI and CommonPool, which are singletons. DefaultDispatcher is also a singleton, so it is named as such. Of course, there is some bike-shedding to be held on how it all shall be named (see also CoroutineDispatcher's names #41), but we'll skip that bike-shedding for now, since the name of DefaultDispatcher is not going to actually appear in the source of end-user applications.

  3. The value. For Kotlin/JVM we'll start with CommonPool as the value of DefaultDispatcher. It is absolutely non-ideal value, since CommonPool is ill suited for the tasks that do not have recursive fork-join structure and most typical coroutine benchmarks run approximately 2 times faster in a single-threaded dispatcher than in CommonPool. Moreover, with CommonPool we cannot efficiently support blocking operations without having to switch to a different thread (see IO thread pool for blocking calls #79). Ultimately, a totally new, engineered from group-up implementation will be needed, but we'll leave this implementation for later.

@elizarov
Copy link
Contributor Author

Commited to develop branch

@DmitriyZaitsev
Copy link
Contributor

Cool! Code became much cleaner, no longer need to pull CommonPool along everywhere.
But does this change mean broken binary compatibility we discussed in #118?

@elizarov
Copy link
Contributor Author

I've added the corresponding bridges manually to maintain binary compatiliby with previously compiled code.

@elizarov
Copy link
Contributor Author

Released in version 0.19

@mslenc
Copy link

mslenc commented Oct 2, 2017

Any chance of having the default dispatcher be configurable by the user? (I mean globally/once, not really in any runtime sense)

My use case is running on Vert.X with basically all builder calls using Unconfined (things are already taken care of by the platform).. With the new default, it's quite easy to accidentally omit the context..

@mslenc
Copy link

mslenc commented Oct 2, 2017

Hi @fvasco,
thanks for the reference, though I'm pretty sure Unconfined does exactly the same in my particular case, as this is never the case..

But regardless, it'd still be very convenient and bug-avoiding if one could set either as the default? :)

@czeidler
Copy link

czeidler commented Feb 8, 2018

I find CommonPool as default dispatcher quite dangerous. IMHO a thread should only be used when really needed but not silently introduced by the framework. I just run into a problem where I thought I'm writing a single threaded application and suddenly I got random crashes because of concurrency issues...

Another issues I see with CommonPool as a default is that it results in inconsistent behaviour in common code (JVM/JS). For example, code that uses launch {} might run fine in JS but crash in JVM because of the introduced concurrency. The default launch implementation should have the same semantics in JVM/JS, e.g. that they use the current CoroutineContext...

@elizarov
Copy link
Contributor Author

elizarov commented Feb 8, 2018

@czeidler The CommonPool is an interim solution for a default dispatcher. In the future we're going to have a different one, but it is still going to be multithreaded on JVM. The JVM is multithreaded and, unlike JS, it does not have "the event loop" that we can use, so you have to always keep JVM multithreading in mind when targeting JVM. However, we plan to provide you addition tools to help find you common multithreading problems in your code.

@mslenc
Copy link

mslenc commented Feb 8, 2018

@elizarov That is all good, yet there are several JVM libraries and frameworks that provide "an event loop" anyway (and they're not in any way obscure either), so please provide that configuration option asked for above, regardless of what the default DefaultDispatcher ends up being..

@czeidler
Copy link

czeidler commented Feb 8, 2018

If there is already a CoroutineContext, can't you just use this CoroutineContext for the new coroutine? This is a bit more conservative and I think also more intuitive.

But yes if there is no event loop/ no CoroutineContext it becomes a bit tricky. A drastic measure would be to throw an Exception if there is no CoroutineContext, i.e. on the JVM you are forced to specify where you want to run your coroutine. Giving it some thought I would actually prefer this solution since not running code is usually better to debug than code that has threading problems.

Good to hear that you are planning some debug tools though.

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

No branches or pull requests

5 participants