Skip to content

Mark BufferedChannelIterator.continuation @BeningDataRace to address … #3873

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

Merged
merged 4 commits into from
Oct 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1582,7 +1582,12 @@ internal open class BufferedChannel<E>(
* When [hasNext] suspends, this field stores the corresponding
* continuation. The [tryResumeHasNext] and [tryResumeHasNextOnClosedChannel]
* function resume this continuation when the [hasNext] invocation should complete.
*
* This property is the subject to bening data race:
* It is nulled-out on both completion and cancellation paths that
* could happen concurrently.
*/
@BenignDataRace
private var continuation: CancellableContinuationImpl<Boolean>? = null

// `hasNext()` is just a special receive operation.
Expand Down
12 changes: 12 additions & 0 deletions kotlinx-coroutines-core/common/src/internal/Concurrent.common.kt
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,15 @@ internal expect class ReentrantLock() {
internal expect inline fun <T> ReentrantLock.withLock(action: () -> T): T

internal expect fun <E> identitySet(expectedSize: Int): MutableSet<E>

/**
* Annotation indicating that the marked property is the subject of benign data race.
* LLVM does not support this notion, so on K/N platforms we alias it into `@Volatile` to prevent potential OoTA.
*
* The purpose of this annotation is not to save an extra-volatile on JVM platform, but rather to explicitly emphasize
* that data-race is benign.
*/
@OptionalExpectation
@Target(AnnotationTarget.FIELD)
@OptIn(ExperimentalMultiplatform::class)
internal expect annotation class BenignDataRace()
16 changes: 13 additions & 3 deletions kotlinx-coroutines-core/common/src/selects/Select.kt
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,12 @@ internal open class SelectImplementation<R>(

/**
* List of clauses waiting on this `select` instance.
*
* This property is the subject to bening data race: concurrent cancellation might null-out this property
* while [trySelect] operation reads it and iterates over its content.
* A logical race is resolved by the consensus on [state] property.
*/
@BenignDataRace
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the explanation here, for example.

private var clauses: MutableList<ClauseData>? = ArrayList(2)

/**
Expand Down Expand Up @@ -407,7 +412,13 @@ internal open class SelectImplementation<R>(
* one that stores either result when the clause is successfully registered ([inRegistrationPhase] is `true`),
* or [DisposableHandle] instance when the clause is completed during registration ([inRegistrationPhase] is `false`).
* Yet, this optimization is omitted for code simplicity.
*
* This property is the subject to benign data race:
* [Cleanup][cleanup] procedure can be invoked both as part of the completion sequence
* and as a cancellation handler triggered by an external cancellation.
* In both scenarios, [NO_RESULT] is written to this property via race.
*/
@BenignDataRace
private var internalResult: Any? = NO_RESULT

/**
Expand Down Expand Up @@ -621,9 +632,8 @@ internal open class SelectImplementation<R>(
// try to resume the continuation.
this.internalResult = internalResult
if (cont.tryResume(onCancellation)) return TRY_SELECT_SUCCESSFUL
// If the resumption failed, we need to clean
// the [result] field to avoid memory leaks.
this.internalResult = null
// If the resumption failed, we need to clean the [result] field to avoid memory leaks.
this.internalResult = NO_RESULT
return TRY_SELECT_CANCELLED
}
}
Expand Down
2 changes: 2 additions & 0 deletions kotlinx-coroutines-core/native/src/internal/Concurrent.kt
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,5 @@ internal open class SuppressSupportingThrowableImpl : Throwable() {
}
}

@Suppress("ACTUAL_WITHOUT_EXPECT") // This suppress can be removed in 2.0: KT-59355
internal actual typealias BenignDataRace = kotlin.concurrent.Volatile