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

RoomlifecycleManager: Unsubscribe room monitoring as a part of room release #397

Open
sacOO7 opened this issue Nov 13, 2024 · 4 comments
Open

Comments

@sacOO7
Copy link

sacOO7 commented Nov 13, 2024

    for (const feature of this._contributors) {
      feature.channel.offAll();
    }

┆Issue is synchronized with this Jira Task by Unito

@sacOO7
Copy link
Author

sacOO7 commented Nov 13, 2024

Also, I am not sure whether we should turn off those listeners before we start with the Room.release operation.
i.e. After setting status to RoomStatus.Releasing, because I also feel room monitoring shouldn't interfere with room release process.

@sacOO7 sacOO7 changed the title RoomlifecycleManager: Unsubscribe room monitoring after room release RoomlifecycleManager: Unsubscribe room monitoring as a part of room release Nov 13, 2024
@AndyTWF
Copy link
Collaborator

AndyTWF commented Nov 14, 2024

All of the code blocks you point to have a check that causes them to effectively no-op if a release (or any other) operation is in progress. The only exception to this is checking for discontinuity.

@sacOO7
Copy link
Author

sacOO7 commented Nov 15, 2024

Yeah, I agree they cause no-op after release ( I think we listen to detached/failed when release is happening ), but callbacks will still be a part of internal listeners after release and won't be garbage collected.
I think when Room goes into Released state, can remove those listeners.

@sacOO7
Copy link
Author

sacOO7 commented Nov 17, 2024

  • I believe we might like to unsubscribe to room monitoring listeners right before we set

    this._lifecycle.setStatus({ status: RoomStatus.Releasing });

  • Since we are commencing the Release operation, there's no point in listening to discontinuity events, or we emit discontinuity error for each feature specifying release operation in progress.

  • Room release should emit Releasing and Released status which denotes end of current room lifeycle.

  • Release mainly detach each channel. This will emit DETACHED or FAILED channel events and will be captured by room monitoring listeners.

  • Although, we do nothing for Detached, for Failed channel event, we override Released room status with Failed room status as a part of room monitoring and start channel-WindDown again ( depending on how we set _operationInProgress flag -> [RoomLifeCycleManager] _operationInProgress flag incorrectly set #406 )

    if (change.current === RoomStatus.Failed) {
    this._logger.debug('RoomLifecycleManager(); detected channel failure', {
    channel: contributor.channel.name,
    });
    this._clearAllTransientDetachTimeouts();
    this._operationInProgress = true;
    this._lifecycle.setStatus({
    status: RoomStatus.Failed,
    error: change.reason,
    });
    // We'll make a best effort at detaching all the other channels
    this._doChannelWindDown(contributor).catch((error: unknown) => {
    this._logger.error('RoomLifecycleManager(); failed to detach all channels following failure', {
    contributor: contributor.channel.name,
    error,
    });
    });

  • We can also get rid of extra checks as per [CHA-RL3c][RoomLifecycle] Redundant _releaseInProgress check #399 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants