Update attachment download handling while in a call

signalapp · Apr 19, 2024 · d0d49a0 · d0d49a0
1 parent a51962e
commit d0d49a0
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 26 deletions.
diff --git a/ts/jobs/AttachmentDownloadManager.ts b/ts/jobs/AttachmentDownloadManager.ts
@@ -243,7 +243,7 @@ export class AttachmentDownloadManager {
 
     return {
       ...attachment,
-      pending: true,
+      pending: !this.shouldHoldOffOnStartingQueuedJobs(),
     };
   }
 
@@ -300,13 +300,6 @@ export class AttachmentDownloadManager {
         return;
       }
 
-      if (this.isInCall()) {
-        log.info(
-          'AttachmentDownloadManager/_maybeStartJobs: holding off on starting new jobs; in call'
-        );
-        return;
-      }
-
       const numJobsToStart = this.getMaximumNumberOfJobsToStart();
 
       if (numJobsToStart <= 0) {
@@ -323,6 +316,17 @@ export class AttachmentDownloadManager {
         timestamp: Date.now(),
       });
 
+      if (nextJobs.length === 0) {
+        return;
+      }
+
+      if (this.shouldHoldOffOnStartingQueuedJobs()) {
+        log.info(
+          `AttachmentDownloadManager/_maybeStartJobs: holding off on starting ${nextJobs.length} new job(s)`
+        );
+        return;
+      }
+
       // TODO (DESKTOP-6913): if a prioritized job is selected, we will to update the
       // in-memory job with that information so we can handle it differently, including
       // e.g. downloading a thumbnail before the full-size version
@@ -395,6 +399,7 @@ export class AttachmentDownloadManager {
       lastAttemptTimestamp: now,
     });
   }
+
   private getActiveJobCount(): number {
     return this.activeJobs.size;
   }
@@ -428,7 +433,7 @@ export class AttachmentDownloadManager {
     if (this.isJobRunning(job)) {
       const jobIdForLogging = getJobIdForLogging(job);
       log.warn(
-        `attachmentDownloads/_addRunningJob: job ${jobIdForLogging} is already running`
+        `AttachmentDownloadManager/addRunningJob: job ${jobIdForLogging} is already running`
       );
     }
     this.activeJobs.set(this.getJobId(job), {
@@ -452,6 +457,10 @@ export class AttachmentDownloadManager {
     return `${messageId}.${attachmentType}.${digest}`;
   }
 
+  private shouldHoldOffOnStartingQueuedJobs(): boolean {
+    return this.isInCall();
+  }
+
   // Static methods
   static get instance(): AttachmentDownloadManager {
     if (!AttachmentDownloadManager._instance) {
@@ -488,7 +497,7 @@ async function runDownloadAttachmentJob(
   isLastAttempt: boolean
 ): Promise<JobResultType> {
   const jobIdForLogging = getJobIdForLogging(job);
-  const logId = `attachment_downloads/runDownloadAttachmentJob/${jobIdForLogging}`;
+  const logId = `AttachmentDownloadManager/runDownloadAttachmentJob/${jobIdForLogging}`;
 
   const message = await __DEPRECATED$getMessageById(job.messageId);
 
@@ -511,6 +520,7 @@ async function runDownloadAttachmentJob(
       await addAttachmentToMessage(
         message,
         _markAttachmentAsTooBig(job.attachment),
+        logId,
         { type: job.attachmentType }
       );
       return { status: 'finished' };
@@ -520,6 +530,7 @@ async function runDownloadAttachmentJob(
       await addAttachmentToMessage(
         message,
         _markAttachmentAsPermanentlyErrored(job.attachment),
+        logId,
         { type: job.attachmentType }
       );
 
@@ -530,6 +541,7 @@ async function runDownloadAttachmentJob(
       await addAttachmentToMessage(
         message,
         _markAttachmentAsTransientlyErrored(job.attachment),
+        logId,
         { type: job.attachmentType }
       );
       return { status: 'finished' };
@@ -542,6 +554,7 @@ async function runDownloadAttachmentJob(
         ...job.attachment,
         pending: false,
       },
+      logId,
       { type: job.attachmentType }
     );
     return { status: 'retry' };
@@ -561,7 +574,7 @@ async function runDownloadAttachmentJobInner(
   const { messageId, attachment, attachmentType: type } = job;
 
   const jobIdForLogging = getJobIdForLogging(job);
-  const logId = `attachment_downloads/_runDownloadJobInner(${jobIdForLogging})`;
+  const logId = `AttachmentDownloadManager/runDownloadJobInner(${jobIdForLogging})`;
 
   if (!job || !attachment || !messageId) {
     throw new Error(`${logId}: Key information required for job was missing.`);
@@ -590,6 +603,7 @@ async function runDownloadAttachmentJobInner(
   await addAttachmentToMessage(
     message,
     { ...attachment, pending: true },
+    logId,
     { type }
   );
 
@@ -601,6 +615,7 @@ async function runDownloadAttachmentJobInner(
   await addAttachmentToMessage(
     message,
     omit(upgradedAttachment, ['error', 'pending']),
+    logId,
     {
       type,
     }

diff --git a/ts/messageModifiers/AttachmentDownloads.ts b/ts/messageModifiers/AttachmentDownloads.ts
@@ -11,13 +11,14 @@ import { getAttachmentSignature, isDownloaded } from '../types/Attachment';
 export async function addAttachmentToMessage(
   message: MessageModel | null | undefined,
   attachment: AttachmentType,
+  jobLogId: string,
   { type }: { type: AttachmentDownloadJobTypeType }
 ): Promise<void> {
   if (!message) {
     return;
   }
 
-  const logPrefix = `${message.idForLogging()} (type: ${type})`;
+  const logPrefix = `${jobLogId}/addAttachmentToMessage`;
   const attachmentSignature = getAttachmentSignature(attachment);
 
   if (type === 'long-message') {

diff --git a/ts/models/messages.ts b/ts/models/messages.ts
@@ -47,7 +47,6 @@ import { isAciString } from '../util/isAciString';
 import * as reactionUtil from '../reactions/util';
 import * as Errors from '../types/errors';
 import type { AttachmentType } from '../types/Attachment';
-import { isImage, isVideo } from '../types/Attachment';
 import * as MIME from '../types/MIME';
 import { ReadStatus } from '../messages/MessageReadStatus';
 import type { SendStateByConversationId } from '../messages/MessageSendState';
@@ -96,7 +95,6 @@ import {
   isTitleTransitionNotification,
 } from '../state/selectors/message';
 import type { ReactionAttributesType } from '../messageModifiers/Reactions';
-import { isInCall } from '../state/selectors/calling';
 import { ReactionSource } from '../reactions/ReactionSource';
 import * as LinkPreview from '../types/LinkPreview';
 import { SignalService as Proto } from '../protobuf';
@@ -2371,23 +2369,18 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
 
     // Only queue attachments for downloads if this is a story (with additional logic), or
     // if it's either an outgoing message or we've accepted the conversation
-    let shouldDownloadNow = false;
-    const attachments = this.get('attachments') || [];
-    const reduxState = window.reduxStore.getState();
-
+    let shouldQueueForDownload = false;
     if (isStory(this.attributes)) {
-      shouldDownloadNow = await shouldDownloadStory(conversation.attributes);
+      shouldQueueForDownload = await shouldDownloadStory(
+        conversation.attributes
+      );
     } else {
-      const isVisualMediaAndUserInCall =
-        isInCall(reduxState) && (isImage(attachments) || isVideo(attachments));
-
-      shouldDownloadNow =
+      shouldQueueForDownload =
         this.hasAttachmentDownloads() &&
-        (conversation.getAccepted() || isOutgoing(this.attributes)) &&
-        !isVisualMediaAndUserInCall;
+        (conversation.getAccepted() || isOutgoing(this.attributes));
     }
 
-    if (shouldDownloadNow) {
+    if (shouldQueueForDownload) {
       if (shouldUseAttachmentDownloadQueue()) {
         addToAttachmentDownloadQueue(idLog, this);
       } else {