From 457a300c9594dc24ba2f05cc16180112b9e8d0ac Mon Sep 17 00:00:00 2001 From: Timo <16718859+toger5@users.noreply.github.com> Date: Tue, 13 May 2025 22:15:41 +0200 Subject: [PATCH] MatrixRTC: Rename `MembershipConfig` parameters (#4714) * Remove redundant sendDelayedEventAction We do already have the state `hasMemberEvent` that allows to distinguish the two cases. No need to create two dedicated actions. * fix missing return * Make membership manager an event emitter to inform about status updates. - deprecate isJoined (replaced by isActivated) - move Interface types to types.ts * add tests for status updates. * lint * test "reschedules delayed leave event" in case the delayed event gets canceled * review * fix types * prettier * fix legacy membership manager * remove deprecated jitter. * use non deprecated config fields (keep deprecated fields as fallback) * update tests to test non deprecated names * make local NewMembershipManager variable names consistent with config * make LegacyMembershipManger local variables consistent with config * comments and rename `networkErrorLocalRetryMs` -> `networkErrorRetryMs` * review --- spec/unit/matrixrtc/MatrixRTCSession.spec.ts | 4 +- spec/unit/matrixrtc/MembershipManager.spec.ts | 18 +++--- src/matrixrtc/LegacyMembershipManager.ts | 42 +++++++------- src/matrixrtc/MatrixRTCSession.ts | 41 +++++++------ src/matrixrtc/NewMembershipManager.ts | 57 ++++++++++--------- 5 files changed, 87 insertions(+), 75 deletions(-) diff --git a/spec/unit/matrixrtc/MatrixRTCSession.spec.ts b/spec/unit/matrixrtc/MatrixRTCSession.spec.ts index 3508e01a6..423f5f13e 100644 --- a/spec/unit/matrixrtc/MatrixRTCSession.spec.ts +++ b/spec/unit/matrixrtc/MatrixRTCSession.spec.ts @@ -402,10 +402,10 @@ describe("MatrixRTCSession", () => { jest.useRealTimers(); }); - it("uses membershipExpiryTimeout from join config", async () => { + it("uses membershipEventExpiryMs from join config", async () => { const realSetTimeout = setTimeout; jest.useFakeTimers(); - sess!.joinRoomSession([mockFocus], mockFocus, { membershipExpiryTimeout: 60000 }); + sess!.joinRoomSession([mockFocus], mockFocus, { membershipEventExpiryMs: 60000 }); await Promise.race([sentStateEvent, new Promise((resolve) => realSetTimeout(resolve, 500))]); expect(client.sendStateEvent).toHaveBeenCalledWith( mockRoom!.roomId, diff --git a/spec/unit/matrixrtc/MembershipManager.spec.ts b/spec/unit/matrixrtc/MembershipManager.spec.ts index afc339f41..36cb456c7 100644 --- a/spec/unit/matrixrtc/MembershipManager.spec.ts +++ b/spec/unit/matrixrtc/MembershipManager.spec.ts @@ -214,7 +214,7 @@ describe.each([ }); const manager = new TestMembershipManager( { - membershipServerSideExpiryTimeout: 9000, + delayedLeaveEventDelayMs: 9000, }, room, client, @@ -293,9 +293,9 @@ describe.each([ await jest.advanceTimersByTimeAsync(5000); expect(client._unstable_sendDelayedStateEvent).toHaveBeenCalledTimes(2); }); - it("uses membershipServerSideExpiryTimeout from config", () => { + it("uses delayedLeaveEventDelayMs from config", () => { const manager = new TestMembershipManager( - { membershipServerSideExpiryTimeout: 123456 }, + { delayedLeaveEventDelayMs: 123456 }, room, client, () => undefined, @@ -311,9 +311,9 @@ describe.each([ }); }); - it("uses membershipExpiryTimeout from config", async () => { + it("uses membershipEventExpiryMs from config", async () => { const manager = new TestMembershipManager( - { membershipExpiryTimeout: 1234567 }, + { membershipEventExpiryMs: 1234567 }, room, client, () => undefined, @@ -479,9 +479,9 @@ describe.each([ // TODO: Not sure about this name describe("background timers", () => { - it("sends only one keep-alive for delayed leave event per `membershipKeepAlivePeriod`", async () => { + it("sends only one keep-alive for delayed leave event per `delayedLeaveEventRestartMs`", async () => { const manager = new TestMembershipManager( - { membershipKeepAlivePeriod: 10_000, membershipServerSideExpiryTimeout: 30_000 }, + { delayedLeaveEventRestartMs: 10_000, delayedLeaveEventDelayMs: 30_000 }, room, client, () => undefined, @@ -512,7 +512,7 @@ describe.each([ // TODO: Add git commit when we removed it. async function testExpires(expire: number, headroom?: number) { const manager = new TestMembershipManager( - { membershipExpiryTimeout: expire, membershipExpiryTimeoutHeadroom: headroom }, + { membershipEventExpiryMs: expire, membershipEventExpiryHeadroomMs: headroom }, room, client, () => undefined, @@ -733,7 +733,7 @@ describe.each([ const unrecoverableError = jest.fn(); (client._unstable_sendDelayedStateEvent as Mock).mockRejectedValue(new HTTPError("unknown", 501)); const manager = new TestMembershipManager( - { callMemberEventRetryDelayMinimum: 1000, maximumNetworkErrorRetryCount: 7 }, + { networkErrorRetryMs: 1000, maximumNetworkErrorRetryCount: 7 }, room, client, () => undefined, diff --git a/src/matrixrtc/LegacyMembershipManager.ts b/src/matrixrtc/LegacyMembershipManager.ts index 7ba393dbc..691980ad4 100644 --- a/src/matrixrtc/LegacyMembershipManager.ts +++ b/src/matrixrtc/LegacyMembershipManager.ts @@ -64,30 +64,32 @@ export class LegacyMembershipManager implements IMembershipManager { private updateCallMembershipRunning = false; private needCallMembershipUpdate = false; /** - * If the server disallows the configured {@link membershipServerSideExpiryTimeout}, + * If the server disallows the configured {@link delayedLeaveEventDelayMs}, * this stores a delay that the server does allow. */ - private membershipServerSideExpiryTimeoutOverride?: number; + private delayedLeaveEventDelayMsOverride?: number; private disconnectDelayId: string | undefined; - private get callMemberEventRetryDelayMinimum(): number { - return this.joinConfig?.callMemberEventRetryDelayMinimum ?? 3_000; + private get networkErrorRetryMs(): number { + return this.joinConfig?.networkErrorRetryMs ?? this.joinConfig?.callMemberEventRetryDelayMinimum ?? 3_000; } - private get membershipExpiryTimeout(): number { - return this.joinConfig?.membershipExpiryTimeout ?? DEFAULT_EXPIRE_DURATION; - } - private get membershipServerSideExpiryTimeout(): number { + private get membershipEventExpiryMs(): number { return ( - this.membershipServerSideExpiryTimeoutOverride ?? + this.joinConfig?.membershipEventExpiryMs ?? + this.joinConfig?.membershipExpiryTimeout ?? + DEFAULT_EXPIRE_DURATION + ); + } + private get delayedLeaveEventDelayMs(): number { + return ( + this.delayedLeaveEventDelayMsOverride ?? + this.joinConfig?.delayedLeaveEventDelayMs ?? this.joinConfig?.membershipServerSideExpiryTimeout ?? 8_000 ); } - private get membershipKeepAlivePeriod(): number { - return this.joinConfig?.membershipKeepAlivePeriod ?? 5_000; - } - private get callMemberEventRetryJitter(): number { - return this.joinConfig?.callMemberEventRetryJitter ?? 2_000; + private get delayedLeaveEventRestartMs(): number { + return this.joinConfig?.delayedLeaveEventRestartMs ?? this.joinConfig?.membershipKeepAlivePeriod ?? 5_000; } public constructor( @@ -137,7 +139,7 @@ export class LegacyMembershipManager implements IMembershipManager { public join(fociPreferred: Focus[], fociActive?: Focus): void { this.ownFocusActive = fociActive; this.ownFociPreferred = fociPreferred; - this.relativeExpiry = this.membershipExpiryTimeout; + this.relativeExpiry = this.membershipEventExpiryMs; // We don't wait for this, mostly because it may fail and schedule a retry, so this // function returning doesn't really mean anything at all. void this.triggerCallMembershipEventUpdate(); @@ -261,7 +263,7 @@ export class LegacyMembershipManager implements IMembershipManager { this.client._unstable_sendDelayedStateEvent( this.room.roomId, { - delay: this.membershipServerSideExpiryTimeout, + delay: this.delayedLeaveEventDelayMs, }, EventType.GroupCallMemberPrefix, {}, // leave event @@ -278,9 +280,9 @@ export class LegacyMembershipManager implements IMembershipManager { const maxDelayAllowed = e.data["org.matrix.msc4140.max_delay"]; if ( typeof maxDelayAllowed === "number" && - this.membershipServerSideExpiryTimeout > maxDelayAllowed + this.delayedLeaveEventDelayMs > maxDelayAllowed ) { - this.membershipServerSideExpiryTimeoutOverride = maxDelayAllowed; + this.delayedLeaveEventDelayMsOverride = maxDelayAllowed; return prepareDelayedDisconnection(); } } @@ -350,7 +352,7 @@ export class LegacyMembershipManager implements IMembershipManager { } logger.info("Sent updated call member event."); } catch (e) { - const resendDelay = this.callMemberEventRetryDelayMinimum + Math.random() * this.callMemberEventRetryJitter; + const resendDelay = this.networkErrorRetryMs; logger.warn(`Failed to send call member event (retrying in ${resendDelay}): ${e}`); await sleep(resendDelay); await this.triggerCallMembershipEventUpdate(); @@ -358,7 +360,7 @@ export class LegacyMembershipManager implements IMembershipManager { } private scheduleDelayDisconnection(): void { - this.memberEventTimeout = setTimeout(() => void this.delayDisconnection(), this.membershipKeepAlivePeriod); + this.memberEventTimeout = setTimeout(() => void this.delayDisconnection(), this.delayedLeaveEventRestartMs); } private readonly delayDisconnection = async (): Promise => { diff --git a/src/matrixrtc/MatrixRTCSession.ts b/src/matrixrtc/MatrixRTCSession.ts index d4ad66254..0a5ce5bca 100644 --- a/src/matrixrtc/MatrixRTCSession.ts +++ b/src/matrixrtc/MatrixRTCSession.ts @@ -65,7 +65,13 @@ export type MatrixRTCSessionEventHandlerMap = { ) => void; [MatrixRTCSessionEvent.MembershipManagerError]: (error: unknown) => void; }; - +// The names follow these principles: +// - we use the technical term delay if the option is related to delayed events. +// - we use delayedLeaveEvent if the option is related to the delayed leave event. +// - we use membershipEvent if the option is related to the rtc member state event. +// - we use the technical term expiry if the option is related to the expiry field of the membership state event. +// - we use a `MS` postfix if the option is a duration to avoid using words like: +// `time`, `duration`, `delay`, `timeout`... that might be mistaken/confused with technical terms. export interface MembershipConfig { /** * Use the new Manager. @@ -80,6 +86,8 @@ export interface MembershipConfig { * * This is what goes into the m.rtc.member event expiry field and is typically set to a number of hours. */ + membershipEventExpiryMs?: number; + /** @deprecated renamed to `membershipEventExpiryMs`*/ membershipExpiryTimeout?: number; /** @@ -91,36 +99,25 @@ export interface MembershipConfig { * * This value does not have an effect on the value of `SessionMembershipData.expires`. */ + membershipEventExpiryHeadroomMs?: number; + /** @deprecated renamed to `membershipEventExpiryHeadroomMs`*/ membershipExpiryTimeoutHeadroom?: number; - /** - * The period (in milliseconds) with which we check that our membership event still exists on the - * server. If it is not found we create it again. - */ - memberEventCheckPeriod?: number; - - /** - * The minimum delay (in milliseconds) after which we will retry sending the membership event if it - * failed to send. - */ - callMemberEventRetryDelayMinimum?: number; - /** * The timeout (in milliseconds) with which the deleayed leave event on the server is configured. * After this time the server will set the event to the disconnected stat if it has not received a keep-alive from the client. */ + delayedLeaveEventDelayMs?: number; + /** @deprecated renamed to `delayedLeaveEventDelayMs`*/ membershipServerSideExpiryTimeout?: number; /** * The interval (in milliseconds) in which the client will send membership keep-alives to the server. */ + delayedLeaveEventRestartMs?: number; + /** @deprecated renamed to `delayedLeaveEventRestartMs`*/ membershipKeepAlivePeriod?: number; - /** - * @deprecated It should be possible to make it stable without this. - */ - callMemberEventRetryJitter?: number; - /** * The maximum number of retries that the manager will do for delayed event sending/updating and state event sending when a server rate limit has been hit. */ @@ -131,6 +128,14 @@ export interface MembershipConfig { */ maximumNetworkErrorRetryCount?: number; + /** + * The time (in milliseconds) after which we will retry a http request if it + * failed to send due to a network error. (send membership event, send delayed event, restart delayed event...) + */ + networkErrorRetryMs?: number; + /** @deprecated renamed to `networkErrorRetryMs`*/ + callMemberEventRetryDelayMinimum?: number; + /** * If true, use the new to-device transport for sending encryption keys. */ diff --git a/src/matrixrtc/NewMembershipManager.ts b/src/matrixrtc/NewMembershipManager.ts index ddc755d53..266834ddd 100644 --- a/src/matrixrtc/NewMembershipManager.ts +++ b/src/matrixrtc/NewMembershipManager.ts @@ -333,33 +333,38 @@ export class MembershipManager private focusActive?: Focus; // Config: - private membershipServerSideExpiryTimeoutOverride?: number; + private delayedLeaveEventDelayMsOverride?: number; - private get callMemberEventRetryDelayMinimum(): number { - return this.joinConfig?.callMemberEventRetryDelayMinimum ?? 3_000; + private get networkErrorRetryMs(): number { + return this.joinConfig?.networkErrorRetryMs ?? this.joinConfig?.callMemberEventRetryDelayMinimum ?? 3_000; } - private get membershipEventExpiryTimeout(): number { - return this.joinConfig?.membershipExpiryTimeout ?? DEFAULT_EXPIRE_DURATION; - } - private get membershipEventExpiryTimeoutHeadroom(): number { - return this.joinConfig?.membershipExpiryTimeoutHeadroom ?? 5_000; - } - private computeNextExpiryActionTs(iteration: number): number { + private get membershipEventExpiryMs(): number { return ( - this.state.startTime + - this.membershipEventExpiryTimeout * iteration - - this.membershipEventExpiryTimeoutHeadroom + this.joinConfig?.membershipEventExpiryMs ?? + this.joinConfig?.membershipExpiryTimeout ?? + DEFAULT_EXPIRE_DURATION ); } - private get membershipServerSideExpiryTimeout(): number { + private get membershipEventExpiryHeadroomMs(): number { return ( - this.membershipServerSideExpiryTimeoutOverride ?? + this.joinConfig?.membershipEventExpiryHeadroomMs ?? + this.joinConfig?.membershipExpiryTimeoutHeadroom ?? + 5_000 + ); + } + private computeNextExpiryActionTs(iteration: number): number { + return this.state.startTime + this.membershipEventExpiryMs * iteration - this.membershipEventExpiryHeadroomMs; + } + private get delayedLeaveEventDelayMs(): number { + return ( + this.delayedLeaveEventDelayMsOverride ?? + this.joinConfig?.delayedLeaveEventDelayMs ?? this.joinConfig?.membershipServerSideExpiryTimeout ?? 8_000 ); } - private get membershipKeepAlivePeriod(): number { - return this.joinConfig?.membershipKeepAlivePeriod ?? 5_000; + private get delayedLeaveEventRestartMs(): number { + return this.joinConfig?.delayedLeaveEventRestartMs ?? this.joinConfig?.membershipKeepAlivePeriod ?? 5_000; } private get maximumRateLimitRetryCount(): number { return this.joinConfig?.maximumRateLimitRetryCount ?? 10; @@ -432,7 +437,7 @@ export class MembershipManager ._unstable_sendDelayedStateEvent( this.room.roomId, { - delay: this.membershipServerSideExpiryTimeout, + delay: this.delayedLeaveEventDelayMs, }, EventType.GroupCallMemberPrefix, {}, // leave event @@ -447,7 +452,7 @@ export class MembershipManager // due to lack of https://github.com/element-hq/synapse/pull/17810 return createInsertActionUpdate( MembershipActionType.RestartDelayedEvent, - this.membershipKeepAlivePeriod, + this.delayedLeaveEventRestartMs, ); } else { // This action was scheduled because we are in the process of joining @@ -525,7 +530,7 @@ export class MembershipManager this.resetRateLimitCounter(MembershipActionType.RestartDelayedEvent); return createInsertActionUpdate( MembershipActionType.RestartDelayedEvent, - this.membershipKeepAlivePeriod, + this.delayedLeaveEventRestartMs, ); }) .catch((e) => { @@ -579,7 +584,7 @@ export class MembershipManager .sendStateEvent( this.room.roomId, EventType.GroupCallMemberPrefix, - this.makeMyMembership(this.membershipEventExpiryTimeout), + this.makeMyMembership(this.membershipEventExpiryMs), this.stateKey, ) .then(() => { @@ -611,7 +616,7 @@ export class MembershipManager .sendStateEvent( this.room.roomId, EventType.GroupCallMemberPrefix, - this.makeMyMembership(this.membershipEventExpiryTimeout * nextExpireUpdateIteration), + this.makeMyMembership(this.membershipEventExpiryMs * nextExpireUpdateIteration), this.stateKey, ) .then(() => { @@ -697,8 +702,8 @@ export class MembershipManager error.data["org.matrix.msc4140.errcode"] === "M_MAX_DELAY_EXCEEDED" ) { const maxDelayAllowed = error.data["org.matrix.msc4140.max_delay"]; - if (typeof maxDelayAllowed === "number" && this.membershipServerSideExpiryTimeout > maxDelayAllowed) { - this.membershipServerSideExpiryTimeoutOverride = maxDelayAllowed; + if (typeof maxDelayAllowed === "number" && this.delayedLeaveEventDelayMs > maxDelayAllowed) { + this.delayedLeaveEventDelayMsOverride = maxDelayAllowed; } this.logger.warn("Retry sending delayed disconnection event due to server timeout limitations:", error); return true; @@ -769,7 +774,7 @@ export class MembershipManager private actionUpdateFromNetworkErrorRetry(error: unknown, type: MembershipActionType): ActionUpdate | undefined { // "Is a network error"-boundary const retries = this.state.networkErrorRetries.get(type) ?? 0; - const retryDurationString = this.callMemberEventRetryDelayMinimum / 1000 + "s"; + const retryDurationString = this.networkErrorRetryMs / 1000 + "s"; const retryCounterString = "(" + retries + "/" + this.maximumNetworkErrorRetryCount + ")"; if (error instanceof Error && error.name === "AbortError") { this.logger.warn( @@ -819,7 +824,7 @@ export class MembershipManager // retry boundary if (retries < this.maximumNetworkErrorRetryCount) { this.state.networkErrorRetries.set(type, retries + 1); - return createInsertActionUpdate(type, this.callMemberEventRetryDelayMinimum); + return createInsertActionUpdate(type, this.networkErrorRetryMs); } // Failure