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

Implemented copyForChildCoroutine() for ThreadContextElement #2936

Closed
wants to merge 28 commits into from

Conversation

yorickhenning
Copy link
Contributor

Fixes #2839.

I've implemented this twice. In the first diff by adding fun copyForChildCoroutine() = this directly to ThreadContextElement. In the second diff by creating a subtype:

interface CopyableThreadContextElement<S> : ThreadContextElement<S> {
  fun copyForChildCoroutine(): CopyableThreadContextElement<S>
}

Using a subtype avoids creating a new CoroutineContext on launch for contexts that contain non-copyable ThreadContextElements. This is cheaper, but more importantly, it's backwards-compatible for coroutineContext reference equality for any CoroutineContext that uses ThreadContextElement. Directly adding this to ThreadContextElement is a subtle breaking change. I prefer the subtype implementation, but either works for me.

This should prevent successful casts to type SettableFuture, meaning
client code can't access and complete the internal Future without
resorting to reflection..
This allows contexts using non-copyable `ThreadContextElements` to
inherit a reference-equal context without constructing a new one.
This allows contexts using non-copyable `ThreadContextElements` to
inherit a reference-equal context without constructing a new one.
This allows contexts using non-copyable `ThreadContextElements` to
inherit a reference-equal context without constructing a new one.
This allows contexts using non-copyable `ThreadContextElements` to
inherit a reference-equal context without constructing a new one.
@qwwdfsad qwwdfsad self-requested a review October 1, 2021 14:37
Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

The overall changes looks good, we'll bikeshed naming a bit and then will merge the PR

}
}

assertNotSame(inheritedElement, parentElement,
Copy link
Member

Choose a reason for hiding this comment

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

This test fails because you're adding CopyForChildCoroutineElement, but querying MyElement

* Returns [this] if `this` has zero [CopyableThreadContextElement] in it.
*/
private fun CoroutineContext.foldCopiesForChildCoroutine(): CoroutineContext {
val jobElementCount = fold(0) { count, it ->
Copy link
Member

Choose a reason for hiding this comment

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

A bit faster and more compact implementation:

val hasToCopy = fold(false) { result, it ->
    result || it is CopyableThreadContextElement<*>
}

* A coroutine using this mechanism can safely call Java code that assumes it's called using a
* `Thread`.
*/
public interface CopyableThreadContextElement<S> : ThreadContextElement<S> {
Copy link
Member

Choose a reason for hiding this comment

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

Please mark it as @ExperimentalCoroutinesApi

qwwdfsad and others added 16 commits October 14, 2021 00:14
…ate API (Kotlin#2922)

* Mention CoroutineDispatcher.limitedParallelism as an intended replacement
* Prepare the API to sharing with K/N new memory model where we _have_ to have this API

Addresses Kotlin#2919
* Add version file to each module resources
* The approach with "Specification-Version" in Manifest doesn't work because Android merges all JARs into a single resource, trimming all the manifests

Fixes Kotlin#2941
…oroutineExceptionHandler (Kotlin#2840)

This change makes `future` coroutine builder consistent with `java.util.concurrent.FutureTask` which also drops exceptions that happen after successful cancellation.

Fixes Kotlin#2774
Fixes Kotlin#2791
* Update binary compatibility validator
* Fix race in testFuturePropagatesExceptionToParentAfterCancellation
Change the build scripts and the file layout so that
kotlinx-coroutines-test is built on all platforms.
* transformWhile
* awaitClose and ProducerScope (for callbackFlow and channelFlow)
* merge
* runningFold, runningReduce, and scan
….IO unbounded for limited parallelism (Kotlin#2918)

* Introduce CoroutineDispatcher.limitedParallelism for granular concurrency control

* Elastic Dispatchers.IO:

    * Extract Ktor-obsolete API to a separate file for backwards compatibility
    * Make Dispatchers.IO being a slice of unlimited blocking scheduler
    * Make Dispatchers.IO.limitParallelism take slices from the same internal scheduler

Fixes Kotlin#2943
Fixes Kotlin#2919
…Kotlin#2957)

* Eagerly load CoroutineExceptionHandler and load the corresponding service

Partially addresses Kotlin#2552
…ine()`.

This is a `ThreadContextElement` that is copy-constructed when a new coroutine
is created and inherits the context.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants