-
Notifications
You must be signed in to change notification settings - Fork 49
feat: incorporate sds-r into reliable channels #2701
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
fac33e2
211a4da
62aae51
fd357f9
2cc49e1
9d82564
81b21ca
6a5eac2
860ecfa
324ecbf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,8 +18,8 @@ | |||||||||||
| MessageChannel, | ||||||||||||
| MessageChannelEvent, | ||||||||||||
| type MessageChannelOptions, | ||||||||||||
| type ParticipantId, | ||||||||||||
| Message as SdsMessage, | ||||||||||||
| type SenderId, | ||||||||||||
| SyncMessage | ||||||||||||
| } from "@waku/sds"; | ||||||||||||
| import { Logger } from "@waku/utils"; | ||||||||||||
|
|
@@ -36,9 +36,11 @@ | |||||||||||
| const log = new Logger("sdk:reliable-channel"); | ||||||||||||
|
|
||||||||||||
| const DEFAULT_SYNC_MIN_INTERVAL_MS = 30 * 1000; // 30 seconds | ||||||||||||
| const DEFAULT_SYNC_MIN_INTERVAL_WITH_REPAIRS_MS = 10 * 1000; // 10 seconds when repairs pending | ||||||||||||
| const DEFAULT_RETRY_INTERVAL_MS = 30 * 1000; // 30 seconds | ||||||||||||
| const DEFAULT_MAX_RETRY_ATTEMPTS = 10; | ||||||||||||
| const DEFAULT_SWEEP_IN_BUF_INTERVAL_MS = 5 * 1000; | ||||||||||||
| const DEFAULT_SWEEP_REPAIR_INTERVAL_MS = 10 * 1000; // 10 seconds | ||||||||||||
| const DEFAULT_PROCESS_TASK_MIN_ELAPSE_MS = 1000; | ||||||||||||
|
|
||||||||||||
| const IRRECOVERABLE_SENDING_ERRORS: LightPushError[] = [ | ||||||||||||
|
|
@@ -78,6 +80,7 @@ | |||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * How often store queries are done to retrieve missing messages. | ||||||||||||
| * Only applies when retrievalStrategy includes Store ('both' or 'store-only'). | ||||||||||||
| * | ||||||||||||
| * @default 10,000 (10 seconds) | ||||||||||||
| */ | ||||||||||||
|
|
@@ -111,6 +114,17 @@ | |||||||||||
| * @default 1000 (1 second) | ||||||||||||
| */ | ||||||||||||
| processTaskMinElapseMs?: number; | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Strategy for retrieving missing messages. | ||||||||||||
| * - 'both': Use SDS-R peer repair and Store queries in parallel (default) | ||||||||||||
| * - 'sds-r-only': Only use SDS-R peer repair | ||||||||||||
| * - 'store-only': Only use Store queries (legacy behavior) | ||||||||||||
| * - 'none': No automatic retrieval | ||||||||||||
| * | ||||||||||||
| * @default 'both' | ||||||||||||
| */ | ||||||||||||
| retrievalStrategy?: "both" | "sds-r-only" | "store-only" | "none"; | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
|
|
@@ -145,11 +159,17 @@ | |||||||||||
| private syncTimeout: ReturnType<typeof setTimeout> | undefined; | ||||||||||||
| private sweepInBufInterval: ReturnType<typeof setInterval> | undefined; | ||||||||||||
| private readonly sweepInBufIntervalMs: number; | ||||||||||||
| private sweepRepairInterval: ReturnType<typeof setInterval> | undefined; | ||||||||||||
| private processTaskTimeout: ReturnType<typeof setTimeout> | undefined; | ||||||||||||
| private readonly retryManager: RetryManager | undefined; | ||||||||||||
| private readonly missingMessageRetriever?: MissingMessageRetriever<T>; | ||||||||||||
| private readonly queryOnConnect?: QueryOnConnect<T>; | ||||||||||||
| private readonly processTaskMinElapseMs: number; | ||||||||||||
| private readonly retrievalStrategy: | ||||||||||||
| | "both" | ||||||||||||
| | "sds-r-only" | ||||||||||||
| | "store-only" | ||||||||||||
| | "none"; | ||||||||||||
| private _started: boolean; | ||||||||||||
|
|
||||||||||||
| private constructor( | ||||||||||||
|
|
@@ -180,7 +200,7 @@ | |||||||||||
|
|
||||||||||||
| if (node.store) { | ||||||||||||
| this._retrieve = node.store.queryGenerator.bind(node.store); | ||||||||||||
| const peerManagerEvents = (node as any)?.peerManager?.events; | ||||||||||||
|
Check warning on line 203 in packages/sdk/src/reliable_channel/reliable_channel.ts
|
||||||||||||
| if ( | ||||||||||||
| peerManagerEvents !== undefined && | ||||||||||||
| (options?.queryOnConnect ?? true) | ||||||||||||
|
|
@@ -214,7 +234,10 @@ | |||||||||||
| this.processTaskMinElapseMs = | ||||||||||||
| options?.processTaskMinElapseMs ?? DEFAULT_PROCESS_TASK_MIN_ELAPSE_MS; | ||||||||||||
|
|
||||||||||||
| if (this._retrieve) { | ||||||||||||
| this.retrievalStrategy = options?.retrievalStrategy ?? "both"; | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are applying default twice, which is not idea. Make the retrievalStrategy a mandatory argument here, so it can be defaulted only once in |
||||||||||||
|
|
||||||||||||
| // Only enable Store retrieval based on strategy | ||||||||||||
| if (this._retrieve && this.shouldUseStore()) { | ||||||||||||
| this.missingMessageRetriever = new MissingMessageRetriever( | ||||||||||||
| this.decoder, | ||||||||||||
| options?.retrieveFrequencyMs, | ||||||||||||
|
|
@@ -264,12 +287,20 @@ | |||||||||||
| public static async create<T extends IDecodedMessage>( | ||||||||||||
| node: IWaku, | ||||||||||||
| channelId: ChannelId, | ||||||||||||
| senderId: SenderId, | ||||||||||||
| senderId: ParticipantId, | ||||||||||||
| encoder: IEncoder, | ||||||||||||
| decoder: IDecoder<T>, | ||||||||||||
| options?: ReliableChannelOptions | ||||||||||||
| ): Promise<ReliableChannel<T>> { | ||||||||||||
| const sdsMessageChannel = new MessageChannel(channelId, senderId, options); | ||||||||||||
| // Enable SDS-R repair only if retrieval strategy uses it | ||||||||||||
| const retrievalStrategy = options?.retrievalStrategy ?? "both"; | ||||||||||||
| const enableRepair = | ||||||||||||
| retrievalStrategy === "both" || retrievalStrategy === "sds-r-only"; | ||||||||||||
|
|
||||||||||||
| const sdsMessageChannel = new MessageChannel(channelId, senderId, { | ||||||||||||
| ...options, | ||||||||||||
| enableRepair | ||||||||||||
| }); | ||||||||||||
| const messageChannel = new ReliableChannel( | ||||||||||||
| node, | ||||||||||||
| sdsMessageChannel, | ||||||||||||
|
|
@@ -418,6 +449,7 @@ | |||||||||||
| // missing messages or the status of previous outgoing messages | ||||||||||||
| this.messageChannel.pushIncomingMessage(sdsMessage, retrievalHint); | ||||||||||||
|
|
||||||||||||
| // Remove from Store retriever if message was retrieved | ||||||||||||
| this.missingMessageRetriever?.removeMissingMessage(sdsMessage.messageId); | ||||||||||||
|
|
||||||||||||
| if (sdsMessage.content && sdsMessage.content.length > 0) { | ||||||||||||
|
|
@@ -478,6 +510,12 @@ | |||||||||||
| this.setupEventListeners(); | ||||||||||||
| this.restartSync(); | ||||||||||||
| this.startSweepIncomingBufferLoop(); | ||||||||||||
|
|
||||||||||||
| // Only start repair sweep if SDS-R is enabled | ||||||||||||
| if (this.shouldUseSdsR()) { | ||||||||||||
| this.startRepairSweepLoop(); | ||||||||||||
| } | ||||||||||||
|
Comment on lines
+514
to
+517
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if it's better to put the check inside |
||||||||||||
|
|
||||||||||||
| if (this._retrieve) { | ||||||||||||
| this.missingMessageRetriever?.start(); | ||||||||||||
| this.queryOnConnect?.start(); | ||||||||||||
|
|
@@ -490,6 +528,7 @@ | |||||||||||
| this._started = false; | ||||||||||||
| this.stopSync(); | ||||||||||||
| this.stopSweepIncomingBufferLoop(); | ||||||||||||
| this.stopRepairSweepLoop(); | ||||||||||||
| this.missingMessageRetriever?.stop(); | ||||||||||||
| this.queryOnConnect?.stop(); | ||||||||||||
| // TODO unsubscribe | ||||||||||||
|
|
@@ -512,18 +551,60 @@ | |||||||||||
| if (this.sweepInBufInterval) clearInterval(this.sweepInBufInterval); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private startRepairSweepLoop(): void { | ||||||||||||
| this.stopRepairSweepLoop(); | ||||||||||||
| this.sweepRepairInterval = setInterval(() => { | ||||||||||||
| void this.messageChannel | ||||||||||||
| .sweepRepairIncomingBuffer(async (message) => { | ||||||||||||
| // Rebroadcast the repair message | ||||||||||||
| const wakuMessage = { payload: message.encode() }; | ||||||||||||
| const result = await this._send(this.encoder, wakuMessage); | ||||||||||||
| return result.failures.length === 0; | ||||||||||||
| }) | ||||||||||||
| .catch((err) => { | ||||||||||||
| log.error("error encountered when sweeping repair buffer", err); | ||||||||||||
| }); | ||||||||||||
| }, DEFAULT_SWEEP_REPAIR_INTERVAL_MS); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private stopRepairSweepLoop(): void { | ||||||||||||
| if (this.sweepRepairInterval) clearInterval(this.sweepRepairInterval); | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
It's apparently better. |
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private shouldUseStore(): boolean { | ||||||||||||
| return ( | ||||||||||||
| this.retrievalStrategy === "both" || | ||||||||||||
| this.retrievalStrategy === "store-only" | ||||||||||||
| ); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private shouldUseSdsR(): boolean { | ||||||||||||
| return ( | ||||||||||||
| this.retrievalStrategy === "both" || | ||||||||||||
| this.retrievalStrategy === "sds-r-only" | ||||||||||||
| ); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private restartSync(multiplier: number = 1): void { | ||||||||||||
| if (this.syncTimeout) { | ||||||||||||
| clearTimeout(this.syncTimeout); | ||||||||||||
| } | ||||||||||||
| if (this.syncMinIntervalMs) { | ||||||||||||
| const timeoutMs = this.random() * this.syncMinIntervalMs * multiplier; | ||||||||||||
| // Adaptive sync: use shorter interval when repairs are pending | ||||||||||||
| const hasPendingRepairs = | ||||||||||||
| this.shouldUseSdsR() && this.messageChannel.hasPendingRepairRequests(); | ||||||||||||
| const baseInterval = hasPendingRepairs | ||||||||||||
| ? DEFAULT_SYNC_MIN_INTERVAL_WITH_REPAIRS_MS | ||||||||||||
| : this.syncMinIntervalMs; | ||||||||||||
|
Comment on lines
+596
to
+598
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WDYT about doing a multiplier of 0.3 or 0.5 instead? |
||||||||||||
|
|
||||||||||||
| const timeoutMs = this.random() * baseInterval * multiplier; | ||||||||||||
|
|
||||||||||||
| this.syncTimeout = setTimeout(() => { | ||||||||||||
| void this.sendSyncMessage(); | ||||||||||||
| // Always restart a sync, no matter whether the message was sent. | ||||||||||||
| // Set a multiplier so we wait a bit longer to not hog the conversation | ||||||||||||
| void this.restartSync(2); | ||||||||||||
| // Use smaller multiplier when repairs pending to send more frequently | ||||||||||||
| const nextMultiplier = hasPendingRepairs ? 1.2 : 2; | ||||||||||||
| void this.restartSync(nextMultiplier); | ||||||||||||
| }, timeoutMs); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
@@ -669,7 +750,13 @@ | |||||||||||
| MessageChannelEvent.InMessageMissing, | ||||||||||||
| (event) => { | ||||||||||||
| for (const { messageId, retrievalHint } of event.detail) { | ||||||||||||
| if (retrievalHint && this.missingMessageRetriever) { | ||||||||||||
| // Store retrieval (for 'both' and 'store-only' strategies) | ||||||||||||
| // SDS-R repair happens automatically via RepairManager for 'both' and 'sds-r-only' | ||||||||||||
| if ( | ||||||||||||
| this.shouldUseStore() && | ||||||||||||
| retrievalHint && | ||||||||||||
| this.missingMessageRetriever | ||||||||||||
| ) { | ||||||||||||
|
Comment on lines
+753
to
+759
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unnecessary, as |
||||||||||||
| this.missingMessageRetriever.addMissingMessage( | ||||||||||||
| messageId, | ||||||||||||
| retrievalHint | ||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am starting the review so it may not hold, but I wonder if instead we should use the "multiplier" concept for this, instead of different values.