Skip to content

Commit

Permalink
fix: remove room from releasing list after release
Browse files Browse the repository at this point in the history
This was missed on the previous iteration, which would occaisionally cause bugs such as channels complaining about
mode/options changes. This change fixes the issue by making sure that release promises don't linger after they're done.

Also fixes React to handle room changes properly w.r.t release.
  • Loading branch information
AndyTWF committed Nov 7, 2024
1 parent da1dbfd commit 4f156f3
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 9 deletions.
5 changes: 4 additions & 1 deletion src/core/rooms.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,10 @@ export class DefaultRooms implements Rooms {
this._rooms.delete(roomId);
const releasePromise = existing.promise.then((room) => {
this._logger.debug('Rooms.release(); releasing room', { roomId, nonce: existing.nonce });
return room.release();
return room.release().then(() => {
this._logger.debug('Rooms.release(); room released', { roomId, nonce: existing.nonce });
this._releasing.delete(roomId);
});
});

this._logger.debug('Rooms.release(); creating new release promise', { roomId, nonce: existing.nonce });
Expand Down
28 changes: 20 additions & 8 deletions src/react/providers/chat-room-provider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ export const ChatRoomProvider: React.FC<ChatRoomProviderProps> = ({
// Create an effect that changes the room when the id or options change
const prevId = useRef(roomId);
const prevOptions = useRef(options);
const prevClient = useRef(client);
const roomReleaseQueue = useRef(new RoomReleaseQueue(logger));

// update the release queue if the logger changes - as it means we have a new client
Expand All @@ -156,28 +157,42 @@ export const ChatRoomProvider: React.FC<ChatRoomProviderProps> = ({
}, [logger]);

useEffect(() => {
if (prevId.current === roomId && prevOptions.current === options) {
logger.debug(`ChatRoomProvider(); running room options change effect`, { roomId, options });
if (client == prevClient.current && prevId.current === roomId && prevOptions.current === options) {
return;
}

prevId.current = roomId;
prevOptions.current = options;
prevClient.current = client;

// The room options or id is changing in some way, so we need to release the old room
// before we create a new one
setValue((prev: ChatRoomContextType) => {
void client.rooms.release(prev.roomId).catch();
logger.debug(`ChatRoomProvider(); running options change setValue`, { roomId, options });
// If we're releasing, release the room - it's guaranteed to be changing
if (release) {
logger.debug(`ChatRoomProvider(); releasing value`, { roomId });
void client.rooms.release(prev.roomId).catch();
} else if (attach) {
// If we're not releasing, but we're attaching, detach the room
void prev.room.then((room) => {
logger.debug(`ChatRoomProvider(); detaching value`, { roomId });
void room.detach().catch();
});
}

logger.debug(`ChatRoomProvider(); updating value`, { roomId, options });
return { room: client.rooms.get(roomId, options).catch(), roomId, options };
});
}, [client, roomId, options, logger]);
}, [client, roomId, options, logger, attach, release]);

// Create an effect that attaches and detaches the room, or releases it when the component unmounts
useEffect(() => {
let unmounted = false;
let resolvedRoom: Room | undefined;
const currentReleaseQueue = roomReleaseQueue.current;
logger.debug(`ChatRoomProvider(); running useEffect`, { roomId: value.roomId });
logger.debug(`ChatRoomProvider(); running attachment and release effect`, { roomId: value.roomId });

// If there was a previous release queued for this room, abort it
currentReleaseQueue.abort(value.roomId, value.options);
Expand Down Expand Up @@ -215,10 +230,7 @@ export const ChatRoomProvider: React.FC<ChatRoomProviderProps> = ({
logger.debug(`ChatRoomProvider(); releasing room`, { roomId: value.roomId });
currentReleaseQueue.enqueue(client, value.roomId, value.options);
return;
}

// If set, detach the room - it's ok to just do this, because the room doesn't get irreversibly disposed
if (resolvedRoom && attach) {
} else if (resolvedRoom && attach) {
logger.debug(`ChatRoomProvider(); detaching room`, { roomId: value.roomId });
void resolvedRoom.detach().catch(() => {
// Ignore, the error will be available via various room status properties
Expand Down
18 changes: 18 additions & 0 deletions test/core/rooms.integration.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { RoomOptionsDefaults } from '@ably/chat';
import * as Ably from 'ably';
import { describe, expect, it } from 'vitest';

Expand Down Expand Up @@ -30,6 +31,23 @@ describe('Rooms', () => {
expect(room).not.toBe(room1);
});

it('releases and recreates a room in cycle', async () => {
// Create a room, then release, then create another room with different options
// We include presence options here because that invokes a change to channel modes - which would flag up
// an error if we were doing releases in the wrong order etc
const chat = newChatClient();
const room1 = await chat.rooms.get('test', { typing: { timeoutMs: 1000 }, presence: RoomOptionsDefaults.presence });
await room1.attach();
await chat.rooms.release('test');

const room2 = await chat.rooms.get('test', { typing: { timeoutMs: 2000 }, presence: RoomOptionsDefaults.presence });
await room2.attach();
await chat.rooms.release('test');

await chat.rooms.get('test', { typing: { timeoutMs: 3000 }, presence: RoomOptionsDefaults.presence });
await chat.rooms.release('test');
});

it('releases a failed room', async () => {
// Create a room, fail it, then release.
const chat = newChatClient();
Expand Down

0 comments on commit 4f156f3

Please sign in to comment.