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

Introduce CloseableCoroutineDispatcher abstract class #2903

Merged
merged 7 commits into from Sep 9, 2021

Conversation

qwwdfsad
Copy link
Member

* Alias it to ExecutorCoroutineDispatcher on JVM as the most appropriate candidate
* Add abstract classes to JVM and Native
* This class will be implemented by new dispathers for K/N new memory model

    * Alias it to ExecutorCoroutineDispatcher on JVM as the most appropriate candidate
    * Add abstract classes to JVM and Native
    * This class will be implemented by new dispathers for K/N new memory model
* **The `CloseableCoroutineDispatcher` class is not stable for inheritance in 3rd party libraries**, as new methods
* might be added to this interface in the future, but is stable for use.
*/
public expect abstract class CloseableCoroutineDispatcher() : CoroutineDispatcher {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also worth checking if we can make it experimental without propagating experimentality to ExecutorCoroutineDispatcher, will do

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Take my words with a grain of salt, as I wasn't on the design meeting for this and it's possible that these questions were already covered)
The main use case for this is #2558, right?

As implemented here, close() is like asking the dispatcher "hey, unload when you're done, okay?" and then just leaving. Is this really the behavior that we want? My assumption is as follows: close() will always be called after the users think they're completely finished working with coroutines. If so, it is unexpected that some jobs are still running or pending at the moment of call to close(). Maybe we could do awaitTermination here. Maybe we could also call shutdownNow and loudly complain if there were some pending tasks. In any case, I think that someone who calls close() on coroutine dispatchers will not be satisfied with just a "best-effort" solution to stop the dispatchers and would want to be notified in some way if there is some rogue job that prevents the code from being unloaded.

* After the successful call to [close], no new tasks will
* be accepted to be [dispatched][dispatch], but previously dispatched tasks will be run.
*
* Invocations of `close` on already closed dispatcher have no effect.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Invocations of `close` on already closed dispatcher have no effect.
* Invocations of `close` on an already closed dispatcher have no effect.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's ambiguity: it is stated above that close initiates the closing sequence, but doesn't necessarily close the dispatcher. I assume the two are considered equivalent here? If so, two ways to rephrase this that I see:

  • Talk here not about closed dispatchers, but only about those "that already entered the closing sequence". Cumbersome, in my opinion.
  • Clarify via a more formal additional sentence that calls to close are idempotent.

@qwwdfsad
Copy link
Member Author

qwwdfsad commented Sep 3, 2021

The main use case for this is #2558, right?

It is not, for that we have another solution, #2915

The main use-case is #2914: we want to introduce newFixedThreadPoolContext() and newSingleThreadContext to concurrent source set (shared between Native and JVM). They represent closeable resources on both platforms, thus we need an abstraction over the dispatcher that represents closeable dispatcher and, well, can be closed.

Apart from that, we already have ExecutorCoroutineDispatcher.close (CloseableCoroutineDispatcher is an alias to that) since the 1.0.0 release and it seems to be accepted quite well, I'm not sure we want to break things here.
awaitTermination cannot be implemented on top of the Executor (only on top of ExecutorService) and it also would be nice to avoid one more public hierarchy for "dispatchers that can only be closed" and "dispatchers that can also be awaited".

qwwdfsad and others added 5 commits September 3, 2021 17:12
…r.kt

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
…r.kt

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
…r.kt

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok. All my points are invalid if unloading the dispatchers is not the point of this.

@qwwdfsad qwwdfsad requested review from dkhalanskyjb and removed request for elizarov September 6, 2021 12:21
@qwwdfsad
Copy link
Member Author

qwwdfsad commented Sep 6, 2021

PTAL one more time just to double-check experimentality

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

@qwwdfsad qwwdfsad merged commit 7523368 into develop Sep 9, 2021
@qwwdfsad qwwdfsad deleted the closeable-dispatcher branch September 9, 2021 12:44
pablobaxter pushed a commit to pablobaxter/kotlinx.coroutines that referenced this pull request Sep 14, 2022
* Introduce CloseableCoroutineDispatcher abstract class
* Alias it to ExecutorCoroutineDispatcher on JVM as the most appropriate candidate
* Add abstract classes to JVM and Native
* This class will be implemented by new dispatchers for K/N new memory model

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
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

Successfully merging this pull request may close these issues.

None yet

2 participants