Base Mutex implementation on Semaphore and improve the latter #3016
Conversation
ndkoval
force-pushed
the
new-channels-mutex
branch
from
84709b8 to
882a827
Compare
|
Only those tests that require |
ndkoval
force-pushed
the
new-channels-mutex
branch
from
882a827 to
9658d3f
Compare
qwwdfsad
left a comment
qwwdfsad
left a comment
There was a problem hiding this comment.
By the way, you can reuse nom-intrusive implementation of onLock: 7755edb#diff-86e8b6879026f347540a4b86b2c594fe25128a0fb168cc0689ef48008c8b4d5fR99
|
@qwwdfsad I already have a proper implementation with new |
qwwdfsad
left a comment
qwwdfsad
left a comment
There was a problem hiding this comment.
I also see that the build is failing, please make it pass. You can temporary ignore onLock tests for that sake.
My biggest concern here is the relevance of the PR -- could you please elaborate on the purpose of the PR? What benefits does it bring?
| affected._state.compareAndSet(this, update) | ||
| override fun resume(value: Unit, onCancellation: ((cause: Throwable) -> Unit)?) { | ||
| this@MutexImpl.owner.value = owner | ||
| cont.resume(value, onCancellation) |
There was a problem hiding this comment.
What will happen if this resume will have no effect, e.g. because the continuation was concurrently cancelled?
| * Use [yield] or [CoroutineScope.isActive] to periodically check for cancellation in tight loops if needed. | ||
| * | ||
| * Use [tryLock] to try acquiring a lock without waiting. | ||
| * This function can be used in [select] invocation with [onLock] clause. |
There was a problem hiding this comment.
This part is irrelevant as onLock is being deprecated for removal
| ) : CancellableContinuation<Unit> by cont { | ||
| override fun tryResume(value: Unit, idempotent: Any?, onCancellation: ((cause: Throwable) -> Unit)?): Any? { | ||
| val token = cont.tryResume(value, idempotent, onCancellation) | ||
| if (token !== null) this@MutexImpl.owner.value = owner |
There was a problem hiding this comment.
Please add a comment why it is safe to have a race here -- the continuation can be already executing and even invoking unlock while owner is not set
There was a problem hiding this comment.
It cannot be executing before completeResume(..)
| override fun toString(): String = "Empty[$locked]" | ||
| } | ||
| internal class MutexImpl(locked: Boolean) : SemaphoreImpl(1, if (locked) 1 else 0), Mutex { | ||
| private val owner = atomic<Any?>(if (locked) null else NO_OWNER) |
There was a problem hiding this comment.
Please document possible state transitions here and their purpose
|
Thanks for the review -- I have fixed and clarified everything in #3020 |
No description provided.