-
Notifications
You must be signed in to change notification settings - Fork 1
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
[ECO-4953][CHA-RL7] Executor for mutually exclusive room lifecycle operations #29
[ECO-4953][CHA-RL7] Executor for mutually exclusive room lifecycle operations #29
Conversation
3e8dca4
to
0596e88
Compare
6009d6c
to
e385614
Compare
0451e23
to
7c05911
Compare
de8cd89
to
e4d0870
Compare
…rstand in the context of given tasks
throw IllegalStateException("Can't perform operation when other operation is going on") | ||
} | ||
operationInProgress = true | ||
delay((200..800).random().toDuration(DurationUnit.MILLISECONDS)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this code does nothing, because all delays inside runTest
are skipped, see
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, runTest
skips use of delay
.
But, that's only true when code is run in the context of a given test coroutineScope.
Since we are passing custom CoroutineScope
, this doesn't skip delay.
You might like to run the test on the local by changing values to say
delay((2000..2100).random().toDuration(DurationUnit.MILLISECONDS))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test will take more time to run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed it, you are right! But nevertheless do we really need any of this delays? Not sure delays make tests more reliable
} | ||
} | ||
|
||
class TaskResult<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's better to use framework's implementation for deferred value - CompletableDeferred
* TaskScheduler schedules given coroutine operation mutually exclusive. | ||
* @property scope Uses single threaded dispatcher to avoid thread synchronization issues. | ||
*/ | ||
class TaskScheduler(private val scope: CoroutineScope) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe implementation of this class implies single-threaded coroutine context, but there is no restriction in public API, which is not very good. Also it's hard to understand how it will be used later. I am a bit skeptical about this scheduler which is pretty low-leveled, I would prefer built-in kotlinx.coroutines
tools. In my head it's something like this
class RoomLifecycleManager {
private val internalEvents = Channel<InternalEvent>(Channel.UNLIMITED)
private val attachmentEvents = Channel<AttachmentEvent>(Channel.UNLIMITED)
private val releasingEvents = Channel<ReleaseEvent>(Channel.UNLIMITED)
init { subscribeToIncommingEvents() }
fun attach() {
subscribeToContributorsStateChange()
dispatch(AttachmentEvent.Attach)
}
fun detach() {
dispatch(AttachmentEvent.Detach)
}
private fun subscribeToIncommingEvents() {
withContext(singleThreadContext) {
while (active) {
select {
internalEvents.onReceive { event ->
processInternalEvent(event)
}
attachmentEvents.onReceive { event ->
processAttachmentEvent(event)
}
releasingEvents.onReceive { event ->
processReleasingEvent(event)
}
}
}
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Official documentation mentions API is experimental -> https://kotlinlang.org/docs/select-expression.html.
Though latest release might have made it stable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will go through the documentation again. This is a good idea to use.
Also, it fits our requirement of prioritized
execution according to the spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will create a new PR for this. We will see how complicated this can get.
The main issue is dealing with while(active)
loop.
We need to keep track of room getting released and if there are any pending events as such : (
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created PR based on suggestion implementation -> #31
…tests accordingly
8fade7d
to
4483533
Compare
be initialized from static method
4483533
to
c3bf5e0
Compare
2c3655a
to
b5a5e54
Compare
This PR is superseded by #32 |
prioritized
execution of operations same as defined in chat-js ( can be changed to simple queue in the future if needed )TODO -