Skip to content

Commit 457a300

Browse files
toger5toger5
authored
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
1 parent be04f00 commit 457a300

File tree

5 files changed

+85
-73
lines changed

5 files changed

+85
-73
lines changed

‎spec/unit/matrixrtc/MatrixRTCSession.spec.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -402,10 +402,10 @@ describe("MatrixRTCSession", () => {
402402
jest.useRealTimers();
403403
});
404404

405-
it("uses membershipExpiryTimeout from join config", async () => {
405+
it("uses membershipEventExpiryMs from join config", async () => {
406406
const realSetTimeout = setTimeout;
407407
jest.useFakeTimers();
408-
sess!.joinRoomSession([mockFocus], mockFocus, { membershipExpiryTimeout: 60000 });
408+
sess!.joinRoomSession([mockFocus], mockFocus, { membershipEventExpiryMs: 60000 });
409409
await Promise.race([sentStateEvent, new Promise((resolve) => realSetTimeout(resolve, 500))]);
410410
expect(client.sendStateEvent).toHaveBeenCalledWith(
411411
mockRoom!.roomId,

‎spec/unit/matrixrtc/MembershipManager.spec.ts

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,7 @@ describe.each([
214214
});
215215
const manager = new TestMembershipManager(
216216
{
217-
membershipServerSideExpiryTimeout: 9000,
217+
delayedLeaveEventDelayMs: 9000,
218218
},
219219
room,
220220
client,
@@ -293,9 +293,9 @@ describe.each([
293293
await jest.advanceTimersByTimeAsync(5000);
294294
expect(client._unstable_sendDelayedStateEvent).toHaveBeenCalledTimes(2);
295295
});
296-
it("uses membershipServerSideExpiryTimeout from config", () => {
296+
it("uses delayedLeaveEventDelayMs from config", () => {
297297
const manager = new TestMembershipManager(
298-
{ membershipServerSideExpiryTimeout: 123456 },
298+
{ delayedLeaveEventDelayMs: 123456 },
299299
room,
300300
client,
301301
() => undefined,
@@ -311,9 +311,9 @@ describe.each([
311311
});
312312
});
313313

314-
it("uses membershipExpiryTimeout from config", async () => {
314+
it("uses membershipEventExpiryMs from config", async () => {
315315
const manager = new TestMembershipManager(
316-
{ membershipExpiryTimeout: 1234567 },
316+
{ membershipEventExpiryMs: 1234567 },
317317
room,
318318
client,
319319
() => undefined,
@@ -479,9 +479,9 @@ describe.each([
479479

480480
// TODO: Not sure about this name
481481
describe("background timers", () => {
482-
it("sends only one keep-alive for delayed leave event per `membershipKeepAlivePeriod`", async () => {
482+
it("sends only one keep-alive for delayed leave event per `delayedLeaveEventRestartMs`", async () => {
483483
const manager = new TestMembershipManager(
484-
{ membershipKeepAlivePeriod: 10_000, membershipServerSideExpiryTimeout: 30_000 },
484+
{ delayedLeaveEventRestartMs: 10_000, delayedLeaveEventDelayMs: 30_000 },
485485
room,
486486
client,
487487
() => undefined,
@@ -512,7 +512,7 @@ describe.each([
512512
// TODO: Add git commit when we removed it.
513513
async function testExpires(expire: number, headroom?: number) {
514514
const manager = new TestMembershipManager(
515-
{ membershipExpiryTimeout: expire, membershipExpiryTimeoutHeadroom: headroom },
515+
{ membershipEventExpiryMs: expire, membershipEventExpiryHeadroomMs: headroom },
516516
room,
517517
client,
518518
() => undefined,
@@ -733,7 +733,7 @@ describe.each([
733733
const unrecoverableError = jest.fn();
734734
(client._unstable_sendDelayedStateEvent as Mock<any>).mockRejectedValue(new HTTPError("unknown", 501));
735735
const manager = new TestMembershipManager(
736-
{ callMemberEventRetryDelayMinimum: 1000, maximumNetworkErrorRetryCount: 7 },
736+
{ networkErrorRetryMs: 1000, maximumNetworkErrorRetryCount: 7 },
737737
room,
738738
client,
739739
() => undefined,

‎src/matrixrtc/LegacyMembershipManager.ts

Lines changed: 21 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -64,30 +64,32 @@ export class LegacyMembershipManager implements IMembershipManager {
6464
private updateCallMembershipRunning = false;
6565
private needCallMembershipUpdate = false;
6666
/**
67-
* If the server disallows the configured {@link membershipServerSideExpiryTimeout},
67+
* If the server disallows the configured {@link delayedLeaveEventDelayMs},
6868
* this stores a delay that the server does allow.
6969
*/
70-
private membershipServerSideExpiryTimeoutOverride?: number;
70+
private delayedLeaveEventDelayMsOverride?: number;
7171
private disconnectDelayId: string | undefined;
7272

73-
private get callMemberEventRetryDelayMinimum(): number {
74-
return this.joinConfig?.callMemberEventRetryDelayMinimum ?? 3_000;
73+
private get networkErrorRetryMs(): number {
74+
return this.joinConfig?.networkErrorRetryMs ?? this.joinConfig?.callMemberEventRetryDelayMinimum ?? 3_000;
7575
}
76-
private get membershipExpiryTimeout(): number {
77-
return this.joinConfig?.membershipExpiryTimeout ?? DEFAULT_EXPIRE_DURATION;
76+
private get membershipEventExpiryMs(): number {
77+
return (
78+
this.joinConfig?.membershipEventExpiryMs ??
79+
this.joinConfig?.membershipExpiryTimeout ??
80+
DEFAULT_EXPIRE_DURATION
81+
);
7882
}
79-
private get membershipServerSideExpiryTimeout(): number {
83+
private get delayedLeaveEventDelayMs(): number {
8084
return (
81-
this.membershipServerSideExpiryTimeoutOverride ??
85+
this.delayedLeaveEventDelayMsOverride ??
86+
this.joinConfig?.delayedLeaveEventDelayMs ??
8287
this.joinConfig?.membershipServerSideExpiryTimeout ??
8388
8_000
8489
);
8590
}
86-
private get membershipKeepAlivePeriod(): number {
87-
return this.joinConfig?.membershipKeepAlivePeriod ?? 5_000;
88-
}
89-
private get callMemberEventRetryJitter(): number {
90-
return this.joinConfig?.callMemberEventRetryJitter ?? 2_000;
91+
private get delayedLeaveEventRestartMs(): number {
92+
return this.joinConfig?.delayedLeaveEventRestartMs ?? this.joinConfig?.membershipKeepAlivePeriod ?? 5_000;
9193
}
9294

9395
public constructor(
@@ -137,7 +139,7 @@ export class LegacyMembershipManager implements IMembershipManager {
137139
public join(fociPreferred: Focus[], fociActive?: Focus): void {
138140
this.ownFocusActive = fociActive;
139141
this.ownFociPreferred = fociPreferred;
140-
this.relativeExpiry = this.membershipExpiryTimeout;
142+
this.relativeExpiry = this.membershipEventExpiryMs;
141143
// We don't wait for this, mostly because it may fail and schedule a retry, so this
142144
// function returning doesn't really mean anything at all.
143145
void this.triggerCallMembershipEventUpdate();
@@ -261,7 +263,7 @@ export class LegacyMembershipManager implements IMembershipManager {
261263
this.client._unstable_sendDelayedStateEvent(
262264
this.room.roomId,
263265
{
264-
delay: this.membershipServerSideExpiryTimeout,
266+
delay: this.delayedLeaveEventDelayMs,
265267
},
266268
EventType.GroupCallMemberPrefix,
267269
{}, // leave event
@@ -278,9 +280,9 @@ export class LegacyMembershipManager implements IMembershipManager {
278280
const maxDelayAllowed = e.data["org.matrix.msc4140.max_delay"];
279281
if (
280282
typeof maxDelayAllowed === "number" &&
281-
this.membershipServerSideExpiryTimeout > maxDelayAllowed
283+
this.delayedLeaveEventDelayMs > maxDelayAllowed
282284
) {
283-
this.membershipServerSideExpiryTimeoutOverride = maxDelayAllowed;
285+
this.delayedLeaveEventDelayMsOverride = maxDelayAllowed;
284286
return prepareDelayedDisconnection();
285287
}
286288
}
@@ -350,15 +352,15 @@ export class LegacyMembershipManager implements IMembershipManager {
350352
}
351353
logger.info("Sent updated call member event.");
352354
} catch (e) {
353-
const resendDelay = this.callMemberEventRetryDelayMinimum + Math.random() * this.callMemberEventRetryJitter;
355+
const resendDelay = this.networkErrorRetryMs;
354356
logger.warn(`Failed to send call member event (retrying in ${resendDelay}): ${e}`);
355357
await sleep(resendDelay);
356358
await this.triggerCallMembershipEventUpdate();
357359
}
358360
}
359361

360362
private scheduleDelayDisconnection(): void {
361-
this.memberEventTimeout = setTimeout(() => void this.delayDisconnection(), this.membershipKeepAlivePeriod);
363+
this.memberEventTimeout = setTimeout(() => void this.delayDisconnection(), this.delayedLeaveEventRestartMs);
362364
}
363365

364366
private readonly delayDisconnection = async (): Promise<void> => {

‎src/matrixrtc/MatrixRTCSession.ts

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,13 @@ export type MatrixRTCSessionEventHandlerMap = {
6565
) => void;
6666
[MatrixRTCSessionEvent.MembershipManagerError]: (error: unknown) => void;
6767
};
68-
68+
// The names follow these principles:
69+
// - we use the technical term delay if the option is related to delayed events.
70+
// - we use delayedLeaveEvent if the option is related to the delayed leave event.
71+
// - we use membershipEvent if the option is related to the rtc member state event.
72+
// - we use the technical term expiry if the option is related to the expiry field of the membership state event.
73+
// - we use a `MS` postfix if the option is a duration to avoid using words like:
74+
// `time`, `duration`, `delay`, `timeout`... that might be mistaken/confused with technical terms.
6975
export interface MembershipConfig {
7076
/**
7177
* Use the new Manager.
@@ -80,6 +86,8 @@ export interface MembershipConfig {
8086
*
8187
* This is what goes into the m.rtc.member event expiry field and is typically set to a number of hours.
8288
*/
89+
membershipEventExpiryMs?: number;
90+
/** @deprecated renamed to `membershipEventExpiryMs`*/
8391
membershipExpiryTimeout?: number;
8492

8593
/**
@@ -91,36 +99,25 @@ export interface MembershipConfig {
9199
*
92100
* This value does not have an effect on the value of `SessionMembershipData.expires`.
93101
*/
102+
membershipEventExpiryHeadroomMs?: number;
103+
/** @deprecated renamed to `membershipEventExpiryHeadroomMs`*/
94104
membershipExpiryTimeoutHeadroom?: number;
95105

96-
/**
97-
* The period (in milliseconds) with which we check that our membership event still exists on the
98-
* server. If it is not found we create it again.
99-
*/
100-
memberEventCheckPeriod?: number;
101-
102-
/**
103-
* The minimum delay (in milliseconds) after which we will retry sending the membership event if it
104-
* failed to send.
105-
*/
106-
callMemberEventRetryDelayMinimum?: number;
107-
108106
/**
109107
* The timeout (in milliseconds) with which the deleayed leave event on the server is configured.
110108
* After this time the server will set the event to the disconnected stat if it has not received a keep-alive from the client.
111109
*/
110+
delayedLeaveEventDelayMs?: number;
111+
/** @deprecated renamed to `delayedLeaveEventDelayMs`*/
112112
membershipServerSideExpiryTimeout?: number;
113113

114114
/**
115115
* The interval (in milliseconds) in which the client will send membership keep-alives to the server.
116116
*/
117+
delayedLeaveEventRestartMs?: number;
118+
/** @deprecated renamed to `delayedLeaveEventRestartMs`*/
117119
membershipKeepAlivePeriod?: number;
118120

119-
/**
120-
* @deprecated It should be possible to make it stable without this.
121-
*/
122-
callMemberEventRetryJitter?: number;
123-
124121
/**
125122
* 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.
126123
*/
@@ -131,6 +128,14 @@ export interface MembershipConfig {
131128
*/
132129
maximumNetworkErrorRetryCount?: number;
133130

131+
/**
132+
* The time (in milliseconds) after which we will retry a http request if it
133+
* failed to send due to a network error. (send membership event, send delayed event, restart delayed event...)
134+
*/
135+
networkErrorRetryMs?: number;
136+
/** @deprecated renamed to `networkErrorRetryMs`*/
137+
callMemberEventRetryDelayMinimum?: number;
138+
134139
/**
135140
* If true, use the new to-device transport for sending encryption keys.
136141
*/

‎src/matrixrtc/NewMembershipManager.ts

Lines changed: 30 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -333,33 +333,38 @@ export class MembershipManager
333333
private focusActive?: Focus;
334334

335335
// Config:
336-
private membershipServerSideExpiryTimeoutOverride?: number;
336+
private delayedLeaveEventDelayMsOverride?: number;
337337

338-
private get callMemberEventRetryDelayMinimum(): number {
339-
return this.joinConfig?.callMemberEventRetryDelayMinimum ?? 3_000;
338+
private get networkErrorRetryMs(): number {
339+
return this.joinConfig?.networkErrorRetryMs ?? this.joinConfig?.callMemberEventRetryDelayMinimum ?? 3_000;
340340
}
341-
private get membershipEventExpiryTimeout(): number {
342-
return this.joinConfig?.membershipExpiryTimeout ?? DEFAULT_EXPIRE_DURATION;
343-
}
344-
private get membershipEventExpiryTimeoutHeadroom(): number {
345-
return this.joinConfig?.membershipExpiryTimeoutHeadroom ?? 5_000;
341+
private get membershipEventExpiryMs(): number {
342+
return (
343+
this.joinConfig?.membershipEventExpiryMs ??
344+
this.joinConfig?.membershipExpiryTimeout ??
345+
DEFAULT_EXPIRE_DURATION
346+
);
346347
}
347-
private computeNextExpiryActionTs(iteration: number): number {
348+
private get membershipEventExpiryHeadroomMs(): number {
348349
return (
349-
this.state.startTime +
350-
this.membershipEventExpiryTimeout * iteration -
351-
this.membershipEventExpiryTimeoutHeadroom
350+
this.joinConfig?.membershipEventExpiryHeadroomMs ??
351+
this.joinConfig?.membershipExpiryTimeoutHeadroom ??
352+
5_000
352353
);
353354
}
354-
private get membershipServerSideExpiryTimeout(): number {
355+
private computeNextExpiryActionTs(iteration: number): number {
356+
return this.state.startTime + this.membershipEventExpiryMs * iteration - this.membershipEventExpiryHeadroomMs;
357+
}
358+
private get delayedLeaveEventDelayMs(): number {
355359
return (
356-
this.membershipServerSideExpiryTimeoutOverride ??
360+
this.delayedLeaveEventDelayMsOverride ??
361+
this.joinConfig?.delayedLeaveEventDelayMs ??
357362
this.joinConfig?.membershipServerSideExpiryTimeout ??
358363
8_000
359364
);
360365
}
361-
private get membershipKeepAlivePeriod(): number {
362-
return this.joinConfig?.membershipKeepAlivePeriod ?? 5_000;
366+
private get delayedLeaveEventRestartMs(): number {
367+
return this.joinConfig?.delayedLeaveEventRestartMs ?? this.joinConfig?.membershipKeepAlivePeriod ?? 5_000;
363368
}
364369
private get maximumRateLimitRetryCount(): number {
365370
return this.joinConfig?.maximumRateLimitRetryCount ?? 10;
@@ -432,7 +437,7 @@ export class MembershipManager
432437
._unstable_sendDelayedStateEvent(
433438
this.room.roomId,
434439
{
435-
delay: this.membershipServerSideExpiryTimeout,
440+
delay: this.delayedLeaveEventDelayMs,
436441
},
437442
EventType.GroupCallMemberPrefix,
438443
{}, // leave event
@@ -447,7 +452,7 @@ export class MembershipManager
447452
// due to lack of https://github.com/element-hq/synapse/pull/17810
448453
return createInsertActionUpdate(
449454
MembershipActionType.RestartDelayedEvent,
450-
this.membershipKeepAlivePeriod,
455+
this.delayedLeaveEventRestartMs,
451456
);
452457
} else {
453458
// This action was scheduled because we are in the process of joining
@@ -525,7 +530,7 @@ export class MembershipManager
525530
this.resetRateLimitCounter(MembershipActionType.RestartDelayedEvent);
526531
return createInsertActionUpdate(
527532
MembershipActionType.RestartDelayedEvent,
528-
this.membershipKeepAlivePeriod,
533+
this.delayedLeaveEventRestartMs,
529534
);
530535
})
531536
.catch((e) => {
@@ -579,7 +584,7 @@ export class MembershipManager
579584
.sendStateEvent(
580585
this.room.roomId,
581586
EventType.GroupCallMemberPrefix,
582-
this.makeMyMembership(this.membershipEventExpiryTimeout),
587+
this.makeMyMembership(this.membershipEventExpiryMs),
583588
this.stateKey,
584589
)
585590
.then(() => {
@@ -611,7 +616,7 @@ export class MembershipManager
611616
.sendStateEvent(
612617
this.room.roomId,
613618
EventType.GroupCallMemberPrefix,
614-
this.makeMyMembership(this.membershipEventExpiryTimeout * nextExpireUpdateIteration),
619+
this.makeMyMembership(this.membershipEventExpiryMs * nextExpireUpdateIteration),
615620
this.stateKey,
616621
)
617622
.then(() => {
@@ -697,8 +702,8 @@ export class MembershipManager
697702
error.data["org.matrix.msc4140.errcode"] === "M_MAX_DELAY_EXCEEDED"
698703
) {
699704
const maxDelayAllowed = error.data["org.matrix.msc4140.max_delay"];
700-
if (typeof maxDelayAllowed === "number" && this.membershipServerSideExpiryTimeout > maxDelayAllowed) {
701-
this.membershipServerSideExpiryTimeoutOverride = maxDelayAllowed;
705+
if (typeof maxDelayAllowed === "number" && this.delayedLeaveEventDelayMs > maxDelayAllowed) {
706+
this.delayedLeaveEventDelayMsOverride = maxDelayAllowed;
702707
}
703708
this.logger.warn("Retry sending delayed disconnection event due to server timeout limitations:", error);
704709
return true;
@@ -769,7 +774,7 @@ export class MembershipManager
769774
private actionUpdateFromNetworkErrorRetry(error: unknown, type: MembershipActionType): ActionUpdate | undefined {
770775
// "Is a network error"-boundary
771776
const retries = this.state.networkErrorRetries.get(type) ?? 0;
772-
const retryDurationString = this.callMemberEventRetryDelayMinimum / 1000 + "s";
777+
const retryDurationString = this.networkErrorRetryMs / 1000 + "s";
773778
const retryCounterString = "(" + retries + "/" + this.maximumNetworkErrorRetryCount + ")";
774779
if (error instanceof Error && error.name === "AbortError") {
775780
this.logger.warn(
@@ -819,7 +824,7 @@ export class MembershipManager
819824
// retry boundary
820825
if (retries < this.maximumNetworkErrorRetryCount) {
821826
this.state.networkErrorRetries.set(type, retries + 1);
822-
return createInsertActionUpdate(type, this.callMemberEventRetryDelayMinimum);
827+
return createInsertActionUpdate(type, this.networkErrorRetryMs);
823828
}
824829

825830
// Failure

0 commit comments

Comments
 (0)