[CHA-RL3c][RoomLifecycle] Redundant _releaseInProgress check

[sacOO7]

• CHA-RL3c mentions -> If the room is in the RELEASING status, then the operation will return the result of the pending release operation, or throw any exception that it throws
• This has been implemented as a part of

  ably-chat-js/src/core/room-lifecycle-manager.ts

  Lines 799 to 818 in 0e1b523

  	if (this._releaseInProgress) {
  	return new Promise<void>((resolve, reject) => {
  	this._lifecycle.onChangeOnce((change: RoomStatusChange) => {
  	if (change.current === RoomStatus.Released) {
  	resolve();
  	return;
  	}
  	this._logger.error('RoomLifecycleManager.release(); expected a non-attached state', change);
  	reject(
  	new Ably.ErrorInfo(
  	'failed to release room; existing attempt failed',
  	ErrorCodes.PreviousOperationFailed,
  	500,
  	change.error,
  	),
  	);
  	});
  	});
  	}

• But this kinda conflicts with, CHA-RL7 which mentions each operation is mutually exclusive and next operation will only start when previous one finishes.
• That means, we don't need to worry about ongoing release op, since new release operation will only start after the previous release operation has finished.
• This will eliminate the above check along with the _releaseInProgress flag, wdyt

┆Issue is synchronized with this Jira Task by Unito

[sacOO7]

Maybe _releaseInProgress was supposed to serve boolean check for previous rooms.get impl. New async rooms.get don't need it anymore wdyt

[AndyTWF]

Perhaps the spec could be a bit clearer - but I don't think they necessarily conflict here. The spec mentions that the operation will not "commence" until the previous has finished, however - it's quite possible that you could argue that checking whether an operation is already in progress and piggybacking off it could occur outside of this (I'd need to think this through, however).

In fact for some future changes, we might want to make it so that simple checks on room status etc (i.e. the releasing check) could occur outside of the mutually exclusive zone. The only thing that really needs to be mutually exclusive is the part when we start doing channel ops.

_releaseInProgress - I'm not sure why we have it as we could just check for the room status being releasing tbh, it didn't come from anything related to async room getting, or before.

[sacOO7]

Yeah, I mean we don't need _releaseInProgress, because it will always return false when used inside this._mtx.runExclusive block 💁‍♂️
Currently, it's only being used inside this._mtx.runExclusive block

[sacOO7]

Also, it seems this._lifecycle.status === RoomStatus.Releasing will always return false inside this._mtx.runExclusive block, because previous releasing operation will always finish with RoomStatus.Released before next one can start

[AndyTWF]

Also, it seems this._lifecycle.status === RoomStatus.Releasing will always return false inside this._mtx.runExclusive block, because previous releasing operation will always finish with RoomStatus.Released before next one can start

That is true, though I can imagine a world whereby depending on exact implementation, we might want to check the status before we request the mtx and short circuit if we can, and again once we have it (at least in JS where we have single thread execution).

[sacOO7]

I strongly feel that, exposing a new world without mtx, would compromise the integrity of the current roomLifeCycelManager where we proudly say each operation is atomic

Currently, every operation is executed under mtx ( as per CHA-RL7 ), which should remain the same in the future as well, so that there won't be race conditions in other SDKs. There will never be issues with ably-js since it's single-threaded anyway.

Also, having mtx simplifies implementation, like currently we can get rid of _releaseInProgress along with related impl. because mtx makes sure a new operation starts when the previous one finishes. This way implementation is easier to read/ interpret and debug.

Also, I don't feel there is any performance overhead as such, mtx will return a chained promise exposing the same result anyway 💁‍♂️

[sacOO7]

Also, I think we should avoid opening this new world, to ensure the atomicity of each operation and for the sake of other SDKs where we need to deal with concurrency issues due to multithreaded env 😬

[AndyTWF]

I agree that at the moment I don't think there's a strong reason to change what we have - I was just giving an example of how things could potentially change in the future if there was a use-case (is being able to check certain state values prematurely necessarily a bad thing, so long as you re-check them within the mutex, for example).

Re _releaseInProgress - we'll consider this for a future spec update to tidy this up a bit :)

[sacOO7]

Thanks, this is helpful 👍

[sacOO7]

As per #397 (comment), I believe we might like to remove the following checks as a part of _doChannelWindDown

1. this._lifecycle.status === RoomStatus.Releasing will always return false
   

   ably-chat-js/src/core/room-lifecycle-manager.ts

   Lines 642 to 643 in 533c4d0

   	this._lifecycle.status === RoomStatus.Releasing ||
   	this._lifecycle.status === RoomStatus.Released) &&

   
   and
2. this._lifecycle.status !== RoomStatus.Releasing will always return true
   

   ably-chat-js/src/core/room-lifecycle-manager.ts

   Lines 665 to 666 in 533c4d0

   	this._lifecycle.status !== RoomStatus.Releasing &&
   	this._lifecycle.status !== RoomStatus.Released

• Since every operation is atomic (as discussed previously), it should reach a deterministic state. So, ideally, we should not have checks for ongoing operations.
• Also, we already have RoomStatus.Released check as a part of ATTACH and DETACH op. Above checks introduce unnecessary duplication : (

[sacOO7]

@AndyTWF I have raised PR to remove related spec point ably/specification#230. Maybe as per our discussion, having CHA-RL3c will not make sense.

[sacOO7]

We can get rid of a lot of redundant code since it is not needed.

[sacOO7]

Related spec ably/specification#230 has been merged hence related code can be refactored/ completely removed : )