Skip to content

Commit f314a31

Browse files
committed
Fix SOE with select (WIP, DO NOT MERGE)
* onSend/onReceive clauses on the same channel: Instead of StackOverflowError we throw IllegalStateException and leave the channel in the original state. * Fix SOE in select with "opposite channels" stress-test. The fix is based on the sequential numbering of atomic select operation. Deadlock is detected and the operation with the lower sequential number is aborted and restarted (with a larger number). Fixes #504 Fixes #1411
1 parent 007d8d7 commit f314a31

23 files changed

+662
-324
lines changed

binary-compatibility-validator/reference-public-api/kotlinx-coroutines-core.txt

+9-2
Original file line numberDiff line numberDiff line change
@@ -1049,7 +1049,9 @@ public final class kotlinx/coroutines/selects/SelectBuilderImpl : kotlinx/corout
10491049
public fun performAtomicTrySelect (Lkotlinx/coroutines/internal/AtomicDesc;)Ljava/lang/Object;
10501050
public fun resumeSelectCancellableWithException (Ljava/lang/Throwable;)V
10511051
public fun resumeWith (Ljava/lang/Object;)V
1052-
public fun trySelect (Ljava/lang/Object;)Z
1052+
public fun toString ()Ljava/lang/String;
1053+
public fun trySelect ()Z
1054+
public fun trySelectOther (Lkotlinx/coroutines/internal/LockFreeLinkedListNode$PrepareOp;)Ljava/lang/Object;
10531055
}
10541056

10551057
public abstract interface class kotlinx/coroutines/selects/SelectClause0 {
@@ -1070,13 +1072,18 @@ public abstract interface class kotlinx/coroutines/selects/SelectInstance {
10701072
public abstract fun isSelected ()Z
10711073
public abstract fun performAtomicTrySelect (Lkotlinx/coroutines/internal/AtomicDesc;)Ljava/lang/Object;
10721074
public abstract fun resumeSelectCancellableWithException (Ljava/lang/Throwable;)V
1073-
public abstract fun trySelect (Ljava/lang/Object;)Z
1075+
public abstract fun trySelect ()Z
1076+
public abstract fun trySelectOther (Lkotlinx/coroutines/internal/LockFreeLinkedListNode$PrepareOp;)Ljava/lang/Object;
10741077
}
10751078

10761079
public final class kotlinx/coroutines/selects/SelectKt {
10771080
public static final fun select (Lkotlin/jvm/functions/Function1;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
10781081
}
10791082

1083+
public synthetic class kotlinx/coroutines/selects/SelectKtSelectOpSequenceNumberRefVolatile {
1084+
public fun <init> (J)V
1085+
}
1086+
10801087
public final class kotlinx/coroutines/selects/SelectUnbiasedKt {
10811088
public static final fun selectUnbiased (Lkotlin/jvm/functions/Function1;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
10821089
}

kotlinx-coroutines-core/common/src/JobSupport.kt

+4-4
Original file line numberDiff line numberDiff line change
@@ -553,7 +553,7 @@ public open class JobSupport constructor(active: Boolean) : Job, ChildJob, Paren
553553
if (select.isSelected) return
554554
if (state !is Incomplete) {
555555
// already complete -- select result
556-
if (select.trySelect(null)) {
556+
if (select.trySelect()) {
557557
block.startCoroutineUnintercepted(select.completion)
558558
}
559559
return
@@ -1181,7 +1181,7 @@ public open class JobSupport constructor(active: Boolean) : Job, ChildJob, Paren
11811181
if (select.isSelected) return
11821182
if (state !is Incomplete) {
11831183
// already complete -- select result
1184-
if (select.trySelect(null)) {
1184+
if (select.trySelect()) {
11851185
if (state is CompletedExceptionally) {
11861186
select.resumeSelectCancellableWithException(state.cause)
11871187
}
@@ -1362,7 +1362,7 @@ private class SelectJoinOnCompletion<R>(
13621362
private val block: suspend () -> R
13631363
) : JobNode<JobSupport>(job) {
13641364
override fun invoke(cause: Throwable?) {
1365-
if (select.trySelect(null))
1365+
if (select.trySelect())
13661366
block.startCoroutineCancellable(select.completion)
13671367
}
13681368
override fun toString(): String = "SelectJoinOnCompletion[$select]"
@@ -1374,7 +1374,7 @@ private class SelectAwaitOnCompletion<T, R>(
13741374
private val block: suspend (T) -> R
13751375
) : JobNode<JobSupport>(job) {
13761376
override fun invoke(cause: Throwable?) {
1377-
if (select.trySelect(null))
1377+
if (select.trySelect())
13781378
job.selectAwaitCompletion(select, block)
13791379
}
13801380
override fun toString(): String = "SelectAwaitOnCompletion[$select]"

kotlinx-coroutines-core/common/src/channels/AbstractChannel.kt

+56-33
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ internal abstract class AbstractSendChannel<E> : SendChannel<E> {
4646
protected open fun offerInternal(element: E): Any {
4747
while (true) {
4848
val receive = takeFirstReceiveOrPeekClosed() ?: return OFFER_FAILED
49-
val token = receive.tryResumeReceive(element, idempotent = null)
49+
val token = receive.tryResumeReceive(element, null)
5050
if (token != null) {
5151
receive.completeResumeReceive(token)
5252
return receive.offerResult
@@ -56,7 +56,7 @@ internal abstract class AbstractSendChannel<E> : SendChannel<E> {
5656

5757
/**
5858
* Tries to add element to buffer or to queued receiver if select statement clause was not selected yet.
59-
* Return type is `ALREADY_SELECTED | OFFER_SUCCESS | OFFER_FAILED | Closed`.
59+
* Return type is `ALREADY_SELECTED | OFFER_SUCCESS | OFFER_FAILED | RETRY_ATOMIC | Closed`.
6060
* @suppress **This is unstable API and it is subject to change.**
6161
*/
6262
protected open fun offerSelectInternal(element: E, select: SelectInstance<*>): Any {
@@ -352,10 +352,13 @@ internal abstract class AbstractSendChannel<E> : SendChannel<E> {
352352
else -> null
353353
}
354354

355-
override fun validatePrepared(node: ReceiveOrClosed<E>): Boolean {
356-
val token = node.tryResumeReceive(element, idempotent = this) ?: return false
355+
@Suppress("UNCHECKED_CAST")
356+
override fun onPrepare(prepareOp: PrepareOp): Any? {
357+
val affected = prepareOp.affected as ReceiveOrClosed<E> // see "failure" impl
358+
val token = affected.tryResumeReceive(element, prepareOp) ?: return REMOVE_PREPARED
359+
if (token === RETRY_ATOMIC) return RETRY_ATOMIC
357360
resumeToken = token
358-
return true
361+
return null
359362
}
360363
}
361364

@@ -388,6 +391,7 @@ internal abstract class AbstractSendChannel<E> : SendChannel<E> {
388391
when {
389392
offerResult === ALREADY_SELECTED -> return
390393
offerResult === OFFER_FAILED -> {} // retry
394+
offerResult === RETRY_ATOMIC -> {} // retry
391395
offerResult === OFFER_SUCCESS -> {
392396
block.startCoroutineUnintercepted(receiver = this, completion = select.completion)
393397
return
@@ -437,8 +441,8 @@ internal abstract class AbstractSendChannel<E> : SendChannel<E> {
437441
@JvmField val select: SelectInstance<R>,
438442
@JvmField val block: suspend (SendChannel<E>) -> R
439443
) : Send(), DisposableHandle {
440-
override fun tryResumeSend(idempotent: Any?): Any? =
441-
if (select.trySelect(idempotent)) SELECT_STARTED else null
444+
override fun tryResumeSend(otherOp: PrepareOp?): Any? =
445+
select.trySelectOther(otherOp)
442446

443447
override fun completeResumeSend(token: Any) {
444448
assert { token === SELECT_STARTED }
@@ -450,7 +454,7 @@ internal abstract class AbstractSendChannel<E> : SendChannel<E> {
450454
}
451455

452456
override fun resumeSendClosed(closed: Closed<*>) {
453-
if (select.trySelect(null))
457+
if (select.trySelect())
454458
select.resumeSelectCancellableWithException(closed.sendException)
455459
}
456460

@@ -461,7 +465,7 @@ internal abstract class AbstractSendChannel<E> : SendChannel<E> {
461465
@JvmField val element: E
462466
) : Send() {
463467
override val pollResult: Any? get() = element
464-
override fun tryResumeSend(idempotent: Any?): Any? = SEND_RESUMED
468+
override fun tryResumeSend(otherOp: PrepareOp?): Any? = SEND_RESUMED.also { otherOp?.finishPrepare() }
465469
override fun completeResumeSend(token: Any) { assert { token === SEND_RESUMED } }
466470
override fun resumeSendClosed(closed: Closed<*>) {}
467471
}
@@ -495,7 +499,7 @@ internal abstract class AbstractChannel<E> : AbstractSendChannel<E>(), Channel<E
495499
protected open fun pollInternal(): Any? {
496500
while (true) {
497501
val send = takeFirstSendOrPeekClosed() ?: return POLL_FAILED
498-
val token = send.tryResumeSend(idempotent = null)
502+
val token = send.tryResumeSend(null)
499503
if (token != null) {
500504
send.completeResumeSend(token)
501505
return send.pollResult
@@ -505,7 +509,7 @@ internal abstract class AbstractChannel<E> : AbstractSendChannel<E>(), Channel<E
505509

506510
/**
507511
* Tries to remove element from buffer or from queued sender if select statement clause was not selected yet.
508-
* Return type is `ALREADY_SELECTED | E | POLL_FAILED | Closed`
512+
* Return type is `ALREADY_SELECTED | E | POLL_FAILED | RETRY_ATOMIC | Closed`
509513
* @suppress **This is unstable API and it is subject to change.**
510514
*/
511515
protected open fun pollSelectInternal(select: SelectInstance<*>): Any? {
@@ -655,11 +659,13 @@ internal abstract class AbstractChannel<E> : AbstractSendChannel<E>(), Channel<E
655659
}
656660

657661
@Suppress("UNCHECKED_CAST")
658-
override fun validatePrepared(node: Send): Boolean {
659-
val token = node.tryResumeSend(idempotent = this) ?: return false
662+
override fun onPrepare(prepareOp: PrepareOp): Any? {
663+
val affected = prepareOp.affected as Send // see "failure" impl
664+
val token = affected.tryResumeSend(prepareOp) ?: return REMOVE_PREPARED
665+
if (token === RETRY_ATOMIC) return RETRY_ATOMIC
660666
resumeToken = token
661-
pollResult = node.pollResult as E
662-
return true
667+
pollResult = affected.pollResult as E
668+
return null
663669
}
664670
}
665671

@@ -681,6 +687,7 @@ internal abstract class AbstractChannel<E> : AbstractSendChannel<E>(), Channel<E
681687
when {
682688
pollResult === ALREADY_SELECTED -> return
683689
pollResult === POLL_FAILED -> {} // retry
690+
pollResult === RETRY_ATOMIC -> {} // retry
684691
pollResult is Closed<*> -> throw recoverStackTrace(pollResult.receiveException)
685692
else -> {
686693
block.startCoroutineUnintercepted(pollResult as E, select.completion)
@@ -709,9 +716,10 @@ internal abstract class AbstractChannel<E> : AbstractSendChannel<E>(), Channel<E
709716
when {
710717
pollResult === ALREADY_SELECTED -> return
711718
pollResult === POLL_FAILED -> {} // retry
719+
pollResult === RETRY_ATOMIC -> {} // retry
712720
pollResult is Closed<*> -> {
713721
if (pollResult.closeCause == null) {
714-
if (select.trySelect(null))
722+
if (select.trySelect())
715723
block.startCoroutineUnintercepted(null, select.completion)
716724
return
717725
} else {
@@ -746,6 +754,7 @@ internal abstract class AbstractChannel<E> : AbstractSendChannel<E>(), Channel<E
746754
when {
747755
pollResult === ALREADY_SELECTED -> return
748756
pollResult === POLL_FAILED -> {} // retry
757+
pollResult === RETRY_ATOMIC -> {} // retry
749758
pollResult is Closed<*> -> {
750759
block.startCoroutineUnintercepted(ValueOrClosed.closed(pollResult.closeCause), select.completion)
751760
}
@@ -870,7 +879,11 @@ internal abstract class AbstractChannel<E> : AbstractSendChannel<E>(), Channel<E
870879
}
871880

872881
@Suppress("IMPLICIT_CAST_TO_ANY")
873-
override fun tryResumeReceive(value: E, idempotent: Any?): Any? = cont.tryResume(resumeValue(value), idempotent)
882+
override fun tryResumeReceive(value: E, otherOp: PrepareOp?): Any? {
883+
otherOp?.finishPrepare()
884+
return cont.tryResume(resumeValue(value), otherOp?.desc)
885+
}
886+
874887
override fun completeResumeReceive(token: Any) = cont.completeResume(token)
875888
override fun resumeReceiveClosed(closed: Closed<*>) {
876889
when {
@@ -886,15 +899,16 @@ internal abstract class AbstractChannel<E> : AbstractSendChannel<E>(), Channel<E
886899
@JvmField val iterator: Itr<E>,
887900
@JvmField val cont: CancellableContinuation<Boolean>
888901
) : Receive<E>() {
889-
override fun tryResumeReceive(value: E, idempotent: Any?): Any? {
890-
val token = cont.tryResume(true, idempotent)
902+
override fun tryResumeReceive(value: E, otherOp: PrepareOp?): Any? {
903+
otherOp?.finishPrepare()
904+
val token = cont.tryResume(true, otherOp?.desc)
891905
if (token != null) {
892906
/*
893-
When idempotent != null this invocation can be stale and we cannot directly update iterator.result
907+
When otherOp != null this invocation can be stale and we cannot directly update iterator.result
894908
Instead, we save both token & result into a temporary IdempotentTokenValue object and
895909
set iterator result only in completeResumeReceive that is going to be invoked just once
896910
*/
897-
if (idempotent != null) return IdempotentTokenValue(token, value)
911+
if (otherOp != null) return IdempotentTokenValue(token, value)
898912
iterator.result = value
899913
}
900914
return token
@@ -928,8 +942,10 @@ internal abstract class AbstractChannel<E> : AbstractSendChannel<E>(), Channel<E
928942
@JvmField val block: suspend (Any?) -> R,
929943
@JvmField val receiveMode: Int
930944
) : Receive<E>(), DisposableHandle {
931-
override fun tryResumeReceive(value: E, idempotent: Any?): Any? =
932-
if (select.trySelect(idempotent)) (value ?: NULL_VALUE) else null
945+
override fun tryResumeReceive(value: E, otherOp: PrepareOp?): Any? {
946+
val result = select.trySelectOther(otherOp)
947+
return if (result === SELECT_STARTED) value ?: NULL_VALUE else result
948+
}
933949

934950
@Suppress("UNCHECKED_CAST")
935951
override fun completeResumeReceive(token: Any) {
@@ -938,7 +954,7 @@ internal abstract class AbstractChannel<E> : AbstractSendChannel<E>(), Channel<E
938954
}
939955

940956
override fun resumeReceiveClosed(closed: Closed<*>) {
941-
if (!select.trySelect(null)) return
957+
if (!select.trySelect()) return
942958
when (receiveMode) {
943959
RECEIVE_THROWS_ON_CLOSE -> select.resumeSelectCancellableWithException(closed.receiveException)
944960
RECEIVE_RESULT -> block.startCoroutine(ValueOrClosed.closed<R>(closed.closeCause), select.completion)
@@ -985,10 +1001,6 @@ internal val POLL_FAILED: Any = Symbol("POLL_FAILED")
9851001
@SharedImmutable
9861002
internal val ENQUEUE_FAILED: Any = Symbol("ENQUEUE_FAILED")
9871003

988-
@JvmField
989-
@SharedImmutable
990-
internal val SELECT_STARTED: Any = Symbol("SELECT_STARTED")
991-
9921004
@JvmField
9931005
@SharedImmutable
9941006
internal val NULL_VALUE: Symbol = Symbol("NULL_VALUE")
@@ -1012,7 +1024,11 @@ internal typealias Handler = (Throwable?) -> Unit
10121024
*/
10131025
internal abstract class Send : LockFreeLinkedListNode() {
10141026
abstract val pollResult: Any? // E | Closed
1015-
abstract fun tryResumeSend(idempotent: Any?): Any?
1027+
// Returns: null - failure,
1028+
// RETRY_ATOMIC for retry (only when otherOp != null),
1029+
// otherwise token for completeResumeSend
1030+
// Must call otherOp?.finishPrepare() before deciding on result other than RETRY_ATOMIC
1031+
abstract fun tryResumeSend(otherOp: PrepareOp?): Any?
10161032
abstract fun completeResumeSend(token: Any)
10171033
abstract fun resumeSendClosed(closed: Closed<*>)
10181034
}
@@ -1022,7 +1038,11 @@ internal abstract class Send : LockFreeLinkedListNode() {
10221038
*/
10231039
internal interface ReceiveOrClosed<in E> {
10241040
val offerResult: Any // OFFER_SUCCESS | Closed
1025-
fun tryResumeReceive(value: E, idempotent: Any?): Any?
1041+
// Returns: null - failure,
1042+
// RETRY_ATOMIC for retry (only when otherOp != null),
1043+
// otherwise token for completeResumeReceive
1044+
// Must call otherOp?.finishPrepare() before deciding on result other than RETRY_ATOMIC
1045+
fun tryResumeReceive(value: E, otherOp: PrepareOp?): Any?
10261046
fun completeResumeReceive(token: Any)
10271047
}
10281048

@@ -1034,7 +1054,10 @@ internal class SendElement(
10341054
override val pollResult: Any?,
10351055
@JvmField val cont: CancellableContinuation<Unit>
10361056
) : Send() {
1037-
override fun tryResumeSend(idempotent: Any?): Any? = cont.tryResume(Unit, idempotent)
1057+
override fun tryResumeSend(otherOp: PrepareOp?): Any? {
1058+
otherOp?.finishPrepare()
1059+
return cont.tryResume(Unit, otherOp?.desc)
1060+
}
10381061
override fun completeResumeSend(token: Any) = cont.completeResume(token)
10391062
override fun resumeSendClosed(closed: Closed<*>) = cont.resumeWithException(closed.sendException)
10401063
override fun toString(): String = "SendElement($pollResult)"
@@ -1051,9 +1074,9 @@ internal class Closed<in E>(
10511074

10521075
override val offerResult get() = this
10531076
override val pollResult get() = this
1054-
override fun tryResumeSend(idempotent: Any?): Any? = CLOSE_RESUMED
1077+
override fun tryResumeSend(otherOp: PrepareOp?): Any? = CLOSE_RESUMED.also { otherOp?.finishPrepare() }
10551078
override fun completeResumeSend(token: Any) { assert { token === CLOSE_RESUMED } }
1056-
override fun tryResumeReceive(value: E, idempotent: Any?): Any? = CLOSE_RESUMED
1079+
override fun tryResumeReceive(value: E, otherOp: PrepareOp?): Any? = CLOSE_RESUMED.also { otherOp?.finishPrepare() }
10571080
override fun completeResumeReceive(token: Any) { assert { token === CLOSE_RESUMED } }
10581081
override fun resumeSendClosed(closed: Closed<*>) = assert { false } // "Should be never invoked"
10591082
override fun toString(): String = "Closed[$closeCause]"

kotlinx-coroutines-core/common/src/channels/ArrayBroadcastChannel.kt

+5-5
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2016-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
2+
* Copyright 2016-2019 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
33
*/
44

55
package kotlinx.coroutines.channels
@@ -105,7 +105,7 @@ internal class ArrayBroadcastChannel<E>(
105105
val size = this.size
106106
if (size >= capacity) return OFFER_FAILED
107107
// let's try to select sending this element to buffer
108-
if (!select.trySelect(null)) { // :todo: move trySelect completion outside of lock
108+
if (!select.trySelect()) { // :todo: move trySelect completion outside of lock
109109
return ALREADY_SELECTED
110110
}
111111
val tail = this.tail
@@ -163,7 +163,7 @@ internal class ArrayBroadcastChannel<E>(
163163
while (true) {
164164
send = takeFirstSendOrPeekClosed() ?: break // when when no sender
165165
if (send is Closed<*>) break // break when closed for send
166-
token = send!!.tryResumeSend(idempotent = null)
166+
token = send!!.tryResumeSend(null)
167167
if (token != null) {
168168
// put sent element to the buffer
169169
buffer[(tail % capacity).toInt()] = (send as Send).pollResult
@@ -245,7 +245,7 @@ internal class ArrayBroadcastChannel<E>(
245245
// find a receiver for an element
246246
receive = takeFirstReceiveOrPeekClosed() ?: break // break when no one's receiving
247247
if (receive is Closed<*>) break // noting more to do if this sub already closed
248-
token = receive.tryResumeReceive(result as E, idempotent = null)
248+
token = receive.tryResumeReceive(result as E, null)
249249
if (token == null) continue // bail out here to next iteration (see for next receiver)
250250
val subHead = this.subHead
251251
this.subHead = subHead + 1 // retrieved element for this subscriber
@@ -299,7 +299,7 @@ internal class ArrayBroadcastChannel<E>(
299299
result === POLL_FAILED -> { /* just bail out of lock */ }
300300
else -> {
301301
// let's try to select receiving this element from buffer
302-
if (!select.trySelect(null)) { // :todo: move trySelect completion outside of lock
302+
if (!select.trySelect()) { // :todo: move trySelect completion outside of lock
303303
result = ALREADY_SELECTED
304304
} else {
305305
// update subHead after retrieiving element from buffer

0 commit comments

Comments
 (0)