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

CoroutineDispatcher's names #41

Closed
fvasco opened this issue Mar 17, 2017 · 14 comments
Closed

CoroutineDispatcher's names #41

fvasco opened this issue Mar 17, 2017 · 14 comments

Comments

@fvasco
Copy link
Contributor

fvasco commented Mar 17, 2017

CoroutineDispatcher's names are too heterogeneous (CommonPool, Unconfined) and programmer isn't helped to collate these in the same hierarchy.
Other CoroutineDispatchers have too common name (Swing, JavaFX) and are subjects to name clash: Look And Feel engine can also expose "Swing" and "JavaFX" singletons.

My prosose is to define a common suffix like "CoroutineDispatcher", "CDispatcher" or simply "Dispatcher" to grouping names.

I.E.

  1. CommonDispatcher
  2. UnconfinedDispatcher
  3. SwingDispatcher
  4. JavaFXDispatcher
launch(CommonDispatcher) {
    . . .
}
@elizarov
Copy link
Contributor

elizarov commented Apr 5, 2017

I'm not convinced that a potential name clash is actually a problem. Can you give some examples as to where you have encountered it?

The argument against this change is that dispatchers are exclusively used as arguments to various coroutine builders: launch, async, produce, etc. This particular usage context gives enough disambiguation to make any kind of "Dispatcher" suffix superfluos (or so it seems).

@fvasco
Copy link
Contributor Author

fvasco commented Apr 5, 2017

This issue raised on my first approach on coroutines.

If we consider Java's List then we can have ArrayList, LinkedList, EmptyList and so on. It is simple to aggregate all these on a single cap.

"Unbounded" and "Swing" are too heterogeneous names, and the argument for "async" method is "CoroutineContext" not "CoroutineDispatcher", so I can wrongly use "Swing" as a coroutine's L&F.

In simple case the code a trained programmer can understand correctly the statement: produce(Unconfined) ...
I fear that in more complex use case a wrong name can lead to confuse code.

@fvasco
Copy link
Contributor Author

fvasco commented May 6, 2017

Hi @elizarov,
I have a problem naming a Vert.x's dispatcher, following current convention its name should be "Vertx"?

Any suggestion in well accepted, thanks.

@elizarov
Copy link
Contributor

I'd give it a short name, like Vx. However, I'm still keeping this naming issue open to review the naming before we finalise 1.0 version of kotlinx.coroutines. Short names might be good for slide-ware, but it is not that clear-cut for a larger project.

@elizarov
Copy link
Contributor

There is also a related issue of naming various functions that take dispatchers as their arguments. Might renaming run to runOn help? See also discussion here: https://discuss.kotlinlang.org/t/calling-suspending-function-from-regular-function/3670

@voddan
Copy link

voddan commented Jul 21, 2017

My issue with coroutine dispatchers not being organized in a hierarchy is poor discoverability. For example, if I want to use a coroutine builder, but forgot the spelling of a dispatcher I need, there is currently no better option than reading the documentation.

I propose to introduce a global singleton Dispatcher for serving as a hierarchy root. Then all coroutine dispatchers can be its extension properties val Dispatcher.Unconfined get() = ... and be used as Dispatcher.Unconfined.

The cost is that we have to use the prefix. The benefit is that IDE now lists all dispatchers for me.

@elizarov
Copy link
Contributor

@voddan That is an interesting idea. I like it more than just giving them longer names. like XXXDispatcher.

@ScottPierce
Copy link
Contributor

I agree that the coroutine dispatcher naming conflict isn't really an issue. It's somewhat nice that it's short and sweet for something that will be commonly used: launch(UI) { ... }

@elizarov
Copy link
Contributor

Closing this issue. We'll leave the general naming approach as it is (a few top-level names).

@elizarov
Copy link
Contributor

elizarov commented Sep 9, 2018

I've reopened this discussion in light of structured concurrency (#410) and the proposed change for the Android dispatcher (#533). The goal is to address two concerns:

  • Discoverability of different dispatchers.
  • Readability of the code, given the fact that dispatcher is always passed as context parameter and there are different types of context element, so it may not be visually obvious which one is a dispatcher.

The concrete proposal I have on the table is to group all the dispatcher references under Dispatchers object:

  • Dispatchers.Default — a default dispatcher for background asynchronous tasks (currently backed by CommonPool, a new dispatcher in the future).
  • Dispatchers.IO — a dispatcher for blocking background operations (IO thread pool for blocking calls #79).
  • Dispatchers.Main — a dispatcher for Android Main Thread (The name for the main Android dispatcher #533).
  • Dispatchers.Swing — a dispatcher for Swing Event Dispatch Thread.
  • Dispatchers.JavaFx — a dispatcher for JavaFx Application Thread.

@chrisbanes
Copy link

I think Dispatchers.Main is going to confuse people. Maybe Dispatchers.Android.Main to make it clearer?

@LouisCAD
Copy link
Contributor

While Dispatchers.Android.Main is a bit more verbose, it is more consistent against Dispatchers.JavaFx and similar, and allows for Dispatchers.Android.UI extension to access the default UI thread with vsync if needed (see #507).

As LifecycleOwners (Activity, Service, etc) are probably going to be CoroutineScopes by themselves, they would default to Dispatchers.Android.Main, so you'd most likely never have to write it by yourself in your codebase.

@elizarov
Copy link
Contributor

@chrisbanes Let me clarify, that Dispatchers.Main is going to be available only when you program for Android and add kotlinx-coroutines-android dependency, so there should not be any confusion about which Main is that. On the other hand, both JavaFx and Swing can be available when you program for Java Desktop, for example, that is why they are named in a more explicit way.

elizarov added a commit that referenced this issue Sep 11, 2018
* Dispatchers.Default — a default dispatcher for background asynchronous tasks
  (currently backed by FJP commonPool, a new dispatcher in the future).
* Dispatchers.IO — a dispatcher for blocking background operations (#79).
* Dispatchers.Main — a dispatcher for Android Main Thread (#533).
* Dispatchers.Swing — a dispatcher for Swing Event Dispatch Thread.
* Dispatchers.JavaFx — a dispatcher for JavaFx Application Thread.
* Old dispatchers are deprecated, CommonPool is deprecated, too.
* awaitPulse() in JavaFx and awaitFrame() in Android are top-level funs.
* Introduced HandlerDispatcher, SwingDispatcher, and JavaFxDispatcher types
  in the corresponding UI modules for type-safety and future extensions

Fixes #41
elizarov added a commit that referenced this issue Sep 11, 2018
* Dispatchers.Default — a default dispatcher for background asynchronous tasks
  (currently backed by FJP commonPool, a new dispatcher in the future).
* Dispatchers.IO — a dispatcher for blocking background operations (#79).
* Dispatchers.Main — a dispatcher for Android Main Thread (#533).
* Dispatchers.Swing — a dispatcher for Swing Event Dispatch Thread.
* Dispatchers.JavaFx — a dispatcher for JavaFx Application Thread.
* Old dispatchers are deprecated, CommonPool is deprecated, too.
* awaitPulse() in JavaFx and awaitFrame() in Android are top-level funs.
* Introduced HandlerDispatcher, SwingDispatcher, and JavaFxDispatcher types
  in the corresponding UI modules for type-safety and future extensions

Fixes #41
Fixes #533
elizarov added a commit that referenced this issue Sep 11, 2018
* Dispatchers.Default — a default dispatcher for background asynchronous tasks
  (currently backed by FJP commonPool, a new dispatcher in the future).
* Dispatchers.IO — a dispatcher for blocking background operations (#79).
* Dispatchers.Main — a dispatcher for Android Main Thread (#533).
* Dispatchers.Swing — a dispatcher for Swing Event Dispatch Thread.
* Dispatchers.JavaFx — a dispatcher for JavaFx Application Thread.
* Old dispatchers are deprecated, CommonPool is deprecated, too.
* awaitPulse() in JavaFx and awaitFrame() in Android are top-level funs.
* Introduced HandlerDispatcher, SwingDispatcher, and JavaFxDispatcher types
  in the corresponding UI modules for type-safety and future extensions

Fixes #41
Fixes #533
@chrisbanes
Copy link

I like the flexibility of @LouisCAD's suggestion. Being able to provide both Android.Main (async) and Android.UI (vsync) dispatchers, but I agree with @adamp in #427 (comment) that having two dispatchers would be confusing too.

I still think Dispatchers.Main is an unfortunate name since it's such a generic noun. Maybe Dispatchers.Android to be similar to JavaFx and Swing?

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

No branches or pull requests

6 participants