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

[ECO-4953][CHA-RL7] Executor for mutually exclusive room lifecycle operations #29

Closed

Conversation

sacOO7
Copy link

@sacOO7 sacOO7 commented Oct 17, 2024

  • Part of Chat Room: implement room lifecycle spec #6
  • Currently, chat spec explicitly defines to perform mutually exclusive lifecycle operations.
  • This includes internal retry, attach, detach, resume etc, more might come in the future.
  • Current implementation mimicks prioritized execution of operations same as defined in chat-js ( can be changed to simple queue in the future if needed )
  • Although, this is not mentioned in the spec, aim to is to keep code totally in sync with chat-js.
  • This will also help with testing using UTS where expected behaviour / assertions will be mainly derived from chat-js functionality.

TODO -

  • Need to add tests

@sacOO7 sacOO7 force-pushed the feature/atomic-room-lifecycle-operation branch from 3e8dca4 to 0596e88 Compare October 17, 2024 10:07
@sacOO7 sacOO7 changed the title Executor for performing mutually exclusive room lifecycle operations Executor for mutually exclusive room lifecycle operations Oct 17, 2024
@sacOO7 sacOO7 force-pushed the feature/atomic-room-lifecycle-operation branch from 6009d6c to e385614 Compare October 17, 2024 11:35
@sacOO7 sacOO7 force-pushed the feature/atomic-room-lifecycle-operation branch from 0451e23 to 7c05911 Compare October 18, 2024 07:09
@sacOO7 sacOO7 force-pushed the feature/atomic-room-lifecycle-operation branch from de8cd89 to e4d0870 Compare October 18, 2024 07:39
throw IllegalStateException("Can't perform operation when other operation is going on")
}
operationInProgress = true
delay((200..800).random().toDuration(DurationUnit.MILLISECONDS))
Copy link
Collaborator

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

Copy link
Author

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))

Copy link
Author

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

Copy link
Collaborator

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

@sacOO7 sacOO7 changed the title Executor for mutually exclusive room lifecycle operations [ECO-4953] Executor for mutually exclusive room lifecycle operations Oct 18, 2024
}
}

class TaskResult<T> {
Copy link
Collaborator

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) {
Copy link
Collaborator

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)
                    }
                }
            }
        }
   }
}

Copy link
Author

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

Copy link
Author

@sacOO7 sacOO7 Oct 19, 2024

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

Copy link
Author

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 : (

Copy link
Author

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

@sacOO7 sacOO7 force-pushed the feature/atomic-room-lifecycle-operation branch 4 times, most recently from 8fade7d to 4483533 Compare October 18, 2024 14:51
@sacOO7 sacOO7 force-pushed the feature/atomic-room-lifecycle-operation branch from 4483533 to c3bf5e0 Compare October 18, 2024 14:58
@sacOO7 sacOO7 changed the title [ECO-4953] Executor for mutually exclusive room lifecycle operations [ECO-4953][CHA-RL7] Executor for mutually exclusive room lifecycle operations Oct 18, 2024
@sacOO7
Copy link
Author

sacOO7 commented Oct 22, 2024

This PR is superseded by #32

@sacOO7 sacOO7 closed this Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants