From a32e5cb9d419672392deabc7be1aebb62ee9f585 Mon Sep 17 00:00:00 2001 From: evgeny Date: Fri, 2 Feb 2024 21:04:01 +0000 Subject: [PATCH 1/2] WIP: check on attaching --- test/realtime/channel.test.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/realtime/channel.test.js b/test/realtime/channel.test.js index a079b64062..b0472b05b7 100644 --- a/test/realtime/channel.test.js +++ b/test/realtime/channel.test.js @@ -1684,5 +1684,20 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, helper, async }); }); }); + + it('should not throw exception then run RealtimeChannels.get() with same options', function (done) { + const realtime = helper.AblyRealtime(); + const channel = realtime.channels.get('channel-with-options', { modes: ['PRESENCE'] }); + channel.attach(); + channel.whenState('attaching', function () { + try { + realtime.channels.get('channel-with-options', { modes: ['PRESENCE'] }); + closeAndFinish(done, realtime); + } catch (err) { + closeAndFinish(done, realtime, err); + } + }); + }); + }); }); From 6b5d0830663f988d64a329554fab341ea78d6017 Mon Sep 17 00:00:00 2001 From: evgeny Date: Mon, 5 Feb 2024 11:05:16 +0000 Subject: [PATCH 2/2] fix: `_shouldReattachToSetOptions` implementation `_shouldReattachToSetOptions` used `this.params` and `this.modes` when checking channel options, but these fields are only set after the channel is attached. This created a bug when `RealtimeChannels.get()` is invoked during `attaching` state. --- .eslintrc.js | 12 ++++++------ src/common/lib/client/realtime.ts | 2 +- src/common/lib/client/realtimechannel.ts | 21 ++++++++++++++------- test/realtime/channel.test.js | 1 - 4 files changed, 21 insertions(+), 15 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index e13cf7864b..478a3b8066 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -31,12 +31,12 @@ module.exports = { { files: ["**/*.{ts,tsx}"], rules: { - "comma-dangle": ["error", "only-multiline"], - "@typescript-eslint/no-unused-vars": ["error"], - - // TypeScript already enforces these rules better than any eslint setup can - "no-undef": "off", - "no-dupe-class-members": "off", + "comma-dangle": ["error", "only-multiline"], + "@typescript-eslint/no-unused-vars": ["error", { "varsIgnorePattern": "^_" }], + // TypeScript already enforces these rules better than any eslint setup can + "no-undef": "off", + "no-dupe-class-members": "off", + "no-unused-vars": "off", }, }, { diff --git a/src/common/lib/client/realtime.ts b/src/common/lib/client/realtime.ts index 2098f5643e..a5dc82a916 100644 --- a/src/common/lib/client/realtime.ts +++ b/src/common/lib/client/realtime.ts @@ -149,7 +149,7 @@ class Channels extends EventEmitter { if (!channel) { channel = this.all[name] = new RealtimeChannel(this.realtime, name, channelOptions); } else if (channelOptions) { - if (channel._shouldReattachToSetOptions(channelOptions)) { + if (channel._shouldReattachToSetOptions(channelOptions, channel.channelOptions)) { throw new ErrorInfo( 'Channels.get() cannot be used to set channel options that would cause the channel to reattach. Please, use RealtimeChannel.setOptions() instead.', 40000, diff --git a/src/common/lib/client/realtimechannel.ts b/src/common/lib/client/realtimechannel.ts index 354be04042..802a133072 100644 --- a/src/common/lib/client/realtimechannel.ts +++ b/src/common/lib/client/realtimechannel.ts @@ -135,6 +135,7 @@ class RealtimeChannel extends Channel { } setOptions(options?: API.Types.ChannelOptions, callback?: ErrCallback): void | Promise { + const previousChannelOptions = this.channelOptions; if (!callback) { if (this.rest.options.promises) { return Utils.promisify(this, 'setOptions', arguments); @@ -154,7 +155,7 @@ class RealtimeChannel extends Channel { } Channel.prototype.setOptions.call(this, options); if (this._decodingContext) this._decodingContext.channelOptions = this.channelOptions; - if (this._shouldReattachToSetOptions(options)) { + if (this._shouldReattachToSetOptions(options, previousChannelOptions)) { /* This does not just do _attach(true, null, callback) because that would put us * into the 'attaching' state until we receive the new attached, which is * conceptually incorrect: we are still attached, we just have a pending request to @@ -184,24 +185,25 @@ class RealtimeChannel extends Channel { } } - _shouldReattachToSetOptions(options?: API.Types.ChannelOptions) { + _shouldReattachToSetOptions(options: API.Types.ChannelOptions | undefined, prevOptions: API.Types.ChannelOptions) { if (!(this.state === 'attached' || this.state === 'attaching')) { return false; } if (options?.params) { // Don't check against the `agent` param - it isn't returned in the ATTACHED message - const requestedParams = Object.assign({}, options.params); - delete requestedParams.agent; - if (Object.keys(requestedParams).length !== 0 && !this.params) { + const requestedParams = omitAgent(options.params); + const existingParams = omitAgent(prevOptions.params); + + if (Object.keys(requestedParams).length !== Object.keys(existingParams).length) { return true; } - if (this.params && !Utils.shallowEquals(this.params, requestedParams)) { + if (!Utils.shallowEquals(existingParams, requestedParams)) { return true; } } if (options?.modes) { - if (!this.modes || !Utils.arrEquals(options.modes, this.modes)) { + if (!prevOptions.modes || !Utils.arrEquals(options.modes, prevOptions.modes)) { return true; } } @@ -1052,4 +1054,9 @@ class RealtimeChannel extends Channel { } } +function omitAgent(channelParams?: API.Types.ChannelParams) { + const { agent: _, ...paramsWithoutAgent } = channelParams || {}; + return paramsWithoutAgent; +} + export default RealtimeChannel; diff --git a/test/realtime/channel.test.js b/test/realtime/channel.test.js index b0472b05b7..5e83d2706b 100644 --- a/test/realtime/channel.test.js +++ b/test/realtime/channel.test.js @@ -1698,6 +1698,5 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, helper, async } }); }); - }); });