From f8b2f73f7b1e8fad8c64f16d96a5c334a10ae4cb Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Thu, 4 Aug 2022 13:25:46 +1000 Subject: [PATCH 1/2] Fixed a few issues found during QA Fixed an issue where quotes containing images wouldn't send Fixed an issue where a MessageSend job could get stuck in an infinite retry loop if it had an attachment in an invalid state Fixed an issue where quotes containing non-media files wouldn't contain the correct data Fixed an issue where the quote thumbnail was getting the wrong content mode set Fixed an issue where the local disappearing messages config wasn't getting generated correctly Fixed an issue where the format parameters for the disappearing message info message were the wrong way around in one case Updated the AttachmentUploadJob to try to support images which haven't completed downloading (untested as it's not supported via the UI) --- .../Content Views/QuoteView.swift | 39 ++++++++++--------- .../Database/Models/Attachment.swift | 22 ++++++++++- .../DisappearingMessageConfiguration.swift | 14 +++---- .../Database/Models/Quote.swift | 4 +- .../Jobs/Types/AttachmentUploadJob.swift | 12 ++++-- .../Jobs/Types/MessageSendJob.swift | 12 +++++- .../Quotes/QuotedReplyModel.swift | 2 +- 7 files changed, 71 insertions(+), 34 deletions(-) diff --git a/Session/Conversations/Message Cells/Content Views/QuoteView.swift b/Session/Conversations/Message Cells/Content Views/QuoteView.swift index 2a47a91d9..03aea60b5 100644 --- a/Session/Conversations/Message Cells/Content Views/QuoteView.swift +++ b/Session/Conversations/Message Cells/Content Views/QuoteView.swift @@ -135,25 +135,8 @@ final class QuoteView: UIView { let fallbackImageName: String = (isAudio ? "attachment_audio" : "actionsheet_document_black") let imageView: UIImageView = UIImageView( image: UIImage(named: fallbackImageName)? + .resizedImage(to: CGSize(width: iconSize, height: iconSize))? .withRenderingMode(.alwaysTemplate) - .resizedImage(to: CGSize(width: iconSize, height: iconSize)) - ) - - attachment.thumbnail( - size: .small, - success: { image, _ in - guard Thread.isMainThread else { - DispatchQueue.main.async { - imageView.image = image - imageView.contentMode = .scaleAspectFill - } - return - } - - imageView.image = image - imageView.contentMode = .scaleAspectFill - }, - failure: {} ) imageView.tintColor = .white @@ -171,6 +154,26 @@ final class QuoteView: UIView { (isAudio ? "Audio" : "Document") ) } + + // Generate the thumbnail if needed + if attachment.isVisualMedia { + attachment.thumbnail( + size: .small, + success: { image, _ in + guard Thread.isMainThread else { + DispatchQueue.main.async { + imageView.image = image + imageView.contentMode = .scaleAspectFill + } + return + } + + imageView.image = image + imageView.contentMode = .scaleAspectFill + }, + failure: {} + ) + } } else { mainStackView.addArrangedSubview(lineView) diff --git a/SessionMessagingKit/Database/Models/Attachment.swift b/SessionMessagingKit/Database/Models/Attachment.swift index e0ceef673..a62849bd7 100644 --- a/SessionMessagingKit/Database/Models/Attachment.swift +++ b/SessionMessagingKit/Database/Models/Attachment.swift @@ -717,6 +717,8 @@ extension Attachment { // MARK: - Convenience extension Attachment { + public static let nonMediaQuoteFileId: String = "NON_MEDIA_QUOTE_FILE_ID" + public enum ThumbnailSize { case small case medium @@ -869,7 +871,7 @@ extension Attachment { return existingImage } - public func cloneAsThumbnail() -> Attachment? { + public func cloneAsQuoteThumbnail() -> Attachment? { let cloneId: String = UUID().uuidString let thumbnailName: String = "quoted-thumbnail-\(sourceFilename ?? "null")" @@ -881,7 +883,22 @@ extension Attachment { mimeType: OWSMimeTypeImageJpeg, sourceFilename: thumbnailName ) - else { return nil } + else { + // Non-media files cannot have thumbnails but may be sent as quotes, in these cases we want + // to create an attachment in an 'uploaded' state with a hard-coded file id so the messageSend + // job doesn't try to upload the attachment (we include the original `serverId` as it's + // required for generating the protobuf) + return Attachment( + id: cloneId, + serverId: self.serverId, + variant: self.variant, + state: .uploaded, + contentType: self.contentType, + byteCount: 0, + downloadUrl: Attachment.nonMediaQuoteFileId, + isValid: self.isValid + ) + } // Try generate the thumbnail var thumbnailData: Data? @@ -922,6 +939,7 @@ extension Attachment { return Attachment( id: cloneId, variant: .standard, + state: .downloaded, contentType: OWSMimeTypeImageJpeg, byteCount: UInt(thumbnailData.count), sourceFilename: thumbnailName, diff --git a/SessionMessagingKit/Database/Models/DisappearingMessageConfiguration.swift b/SessionMessagingKit/Database/Models/DisappearingMessageConfiguration.swift index 08937d1e4..6d4b1cfbc 100644 --- a/SessionMessagingKit/Database/Models/DisappearingMessageConfiguration.swift +++ b/SessionMessagingKit/Database/Models/DisappearingMessageConfiguration.swift @@ -79,8 +79,8 @@ public extension DisappearingMessagesConfiguration { return String( format: "OTHER_UPDATED_DISAPPEARING_MESSAGES_CONFIGURATION".localized(), - NSString.formatDurationSeconds(UInt32(floor(durationSeconds)), useShortFormat: false), - senderName + senderName, + NSString.formatDurationSeconds(UInt32(floor(durationSeconds)), useShortFormat: false) ) } } @@ -192,14 +192,14 @@ public class SMKDisappearingMessagesConfiguration: NSObject { return } - let config: DisappearingMessagesConfiguration = (try DisappearingMessagesConfiguration - .fetchOne(db, id: threadId)? + let config: DisappearingMessagesConfiguration = try DisappearingMessagesConfiguration + .fetchOne(db, id: threadId) + .defaulting(to: DisappearingMessagesConfiguration.defaultWith(threadId)) .with( isEnabled: isEnabled, durationSeconds: durationSeconds ) - .saved(db)) - .defaulting(to: DisappearingMessagesConfiguration.defaultWith(threadId)) + .saved(db) let interaction: Interaction = try Interaction( threadId: threadId, @@ -214,7 +214,7 @@ public class SMKDisappearingMessagesConfiguration: NSObject { db, message: ExpirationTimerUpdate( syncTarget: nil, - duration: UInt32(floor(durationSeconds)) + duration: UInt32(floor(isEnabled ? durationSeconds : 0)) ), interactionId: interaction.id, in: thread diff --git a/SessionMessagingKit/Database/Models/Quote.swift b/SessionMessagingKit/Database/Models/Quote.swift index 9ac83853c..633676aa6 100644 --- a/SessionMessagingKit/Database/Models/Quote.swift +++ b/SessionMessagingKit/Database/Models/Quote.swift @@ -113,7 +113,7 @@ public extension Quote { .map { quotedInteraction -> Attachment? in // If the quotedInteraction has an attachment then try clone it if let attachment: Attachment = try? quotedInteraction.attachments.fetchOne(db) { - return attachment.cloneAsThumbnail() + return attachment.cloneAsQuoteThumbnail() } // Otherwise if the quotedInteraction has a link preview, try clone that @@ -121,7 +121,7 @@ public extension Quote { .fetchOne(db)? .attachment .fetchOne(db)? - .cloneAsThumbnail() + .cloneAsQuoteThumbnail() } .defaulting(to: Attachment(proto: attachment)) .inserted(db) diff --git a/SessionMessagingKit/Jobs/Types/AttachmentUploadJob.swift b/SessionMessagingKit/Jobs/Types/AttachmentUploadJob.swift index ae538be47..4692d48b1 100644 --- a/SessionMessagingKit/Jobs/Types/AttachmentUploadJob.swift +++ b/SessionMessagingKit/Jobs/Types/AttachmentUploadJob.swift @@ -43,9 +43,15 @@ public enum AttachmentUploadJob: JobExecutor { } } - // Note: In the AttachmentUploadJob we intentionally don't provide our own db instance to prevent reentrancy - // issues when the success/failure closures get called before the upload as the JobRunner will attempt to - // update the state of the job immediately + // If the attachment is still pending download the hold off on running this job + guard attachment.state != .pendingDownload && attachment.state != .downloading else { + deferred(job) + return + } + + // Note: In the AttachmentUploadJob we intentionally don't provide our own db instance to prevent + // reentrancy issues when the success/failure closures get called before the upload as the JobRunner + // will attempt to update the state of the job immediately attachment.upload( queue: queue, using: { db, data in diff --git a/SessionMessagingKit/Jobs/Types/MessageSendJob.swift b/SessionMessagingKit/Jobs/Types/MessageSendJob.swift index f7bbceb62..8e9aa3732 100644 --- a/SessionMessagingKit/Jobs/Types/MessageSendJob.swift +++ b/SessionMessagingKit/Jobs/Types/MessageSendJob.swift @@ -75,7 +75,17 @@ public enum MessageSendJob: JobExecutor { // but not on the message recipients device - both LinkPreview and Quote can // have this case) try allAttachmentStateInfo - .filter { $0.state == .uploading || $0.state == .failedUpload || $0.state == .downloaded } + .filter { attachment -> Bool in + // Non-media quotes won't have thumbnails so so don't try to upload them + guard attachment.downloadUrl != Attachment.nonMediaQuoteFileId else { return false } + + switch attachment.state { + case .uploading, .pendingDownload, .downloading, .failedUpload, .downloaded: + return true + + default: return false + } + } .filter { stateInfo in // Don't add a new job if there is one already in the queue !JobRunner.hasPendingOrRunningJob( diff --git a/SessionMessagingKit/Sending & Receiving/Quotes/QuotedReplyModel.swift b/SessionMessagingKit/Sending & Receiving/Quotes/QuotedReplyModel.swift index abde118f3..9e688faaa 100644 --- a/SessionMessagingKit/Sending & Receiving/Quotes/QuotedReplyModel.swift +++ b/SessionMessagingKit/Sending & Receiving/Quotes/QuotedReplyModel.swift @@ -69,7 +69,7 @@ public extension QuotedReplyModel { guard let sourceAttachment: Attachment = self.attachment else { return nil } return try sourceAttachment - .cloneAsThumbnail()? + .cloneAsQuoteThumbnail()? .inserted(db) .id } From ecbded38199c9a7e302c42d8fda35e0ba5fc7283 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Thu, 4 Aug 2022 18:09:03 +1000 Subject: [PATCH 2/2] Cleaned up the poller logic a bit --- .../Pollers/ClosedGroupPoller.swift | 68 ++++++++---------- .../Sending & Receiving/Pollers/Poller.swift | 71 +++++++++---------- SessionUtilitiesKit/JobRunner/JobRunner.swift | 2 + 3 files changed, 65 insertions(+), 76 deletions(-) diff --git a/SessionMessagingKit/Sending & Receiving/Pollers/ClosedGroupPoller.swift b/SessionMessagingKit/Sending & Receiving/Pollers/ClosedGroupPoller.swift index 71c4f15a5..f747a1cc2 100644 --- a/SessionMessagingKit/Sending & Receiving/Pollers/ClosedGroupPoller.swift +++ b/SessionMessagingKit/Sending & Receiving/Pollers/ClosedGroupPoller.swift @@ -184,31 +184,21 @@ public final class ClosedGroupPoller { guard isBackgroundPoll || poller?.isPolling.wrappedValue[groupPublicKey] == true else { return Promise.value(()) } var promises: [Promise] = [] - var messageCount: Int = 0 - let totalMessagesCount: Int = messageResults - .map { result -> Int in - switch result { - case .fulfilled(let messages): return messages.count - default: return 0 + let allMessages: [SnodeReceivedMessage] = messageResults + .reduce([]) { result, next in + switch next { + case .fulfilled(let messages): return result.appending(contentsOf: messages) + default: return result } } - .reduce(0, +) + var messageCount: Int = 0 + let totalMessagesCount: Int = allMessages.count - messageResults.forEach { result in - guard case .fulfilled(let messages) = result else { return } - guard !messages.isEmpty else { return } - - var jobToRun: Job? - - Storage.shared.write { db in - var jobDetailMessages: [MessageReceiveJob.Details.MessageInfo] = [] - - messages.forEach { message in + Storage.shared.write { db in + let processedMessages: [ProcessedMessage] = allMessages + .compactMap { message -> ProcessedMessage? in do { - let processedMessage: ProcessedMessage? = try Message.processRawReceivedMessage(db, rawMessage: message) - - jobDetailMessages = jobDetailMessages - .appending(processedMessage?.messageInfo) + return try Message.processRawReceivedMessage(db, rawMessage: message) } catch { switch error { @@ -219,28 +209,30 @@ public final class ClosedGroupPoller { MessageReceiverError.duplicateControlMessage, MessageReceiverError.selfSend: break - + default: SNLog("Failed to deserialize envelope due to error: \(error).") } + + return nil } } - - messageCount += jobDetailMessages.count - jobToRun = Job( - variant: .messageReceive, - behaviour: .runOnce, - threadId: groupPublicKey, - details: MessageReceiveJob.Details( - messages: jobDetailMessages, - isBackgroundPoll: isBackgroundPoll - ) - ) - - // If we are force-polling then add to the JobRunner so they are persistent and will retry on - // the next app run if they fail but don't let them auto-start - JobRunner.add(db, job: jobToRun, canStartJob: !isBackgroundPoll) - } + messageCount = processedMessages.count + + let jobToRun: Job? = Job( + variant: .messageReceive, + behaviour: .runOnce, + threadId: groupPublicKey, + details: MessageReceiveJob.Details( + messages: processedMessages.map { $0.messageInfo }, + isBackgroundPoll: isBackgroundPoll + ) + ) + + // If we are force-polling then add to the JobRunner so they are persistent and will retry on + // the next app run if they fail but don't let them auto-start + JobRunner.add(db, job: jobToRun, canStartJob: !isBackgroundPoll) + // We want to try to handle the receive jobs immediately in the background if isBackgroundPoll { promises = promises.appending( diff --git a/SessionMessagingKit/Sending & Receiving/Pollers/Poller.swift b/SessionMessagingKit/Sending & Receiving/Pollers/Poller.swift index f63a29486..9e2c0d310 100644 --- a/SessionMessagingKit/Sending & Receiving/Pollers/Poller.swift +++ b/SessionMessagingKit/Sending & Receiving/Pollers/Poller.swift @@ -136,49 +136,44 @@ public final class Poller { var messageCount: Int = 0 Storage.shared.write { db in - var threadMessages: [String: [MessageReceiveJob.Details.MessageInfo]] = [:] - - messages.forEach { message in - do { - let processedMessage: ProcessedMessage? = try Message.processRawReceivedMessage(db, rawMessage: message) - let key: String = (processedMessage?.threadId ?? Message.nonThreadMessageId) - - threadMessages[key] = (threadMessages[key] ?? []) - .appending(processedMessage?.messageInfo) - } - catch { - switch error { - // Ignore duplicate & selfSend message errors (and don't bother logging - // them as there will be a lot since we each service node duplicates messages) - case DatabaseError.SQLITE_CONSTRAINT_UNIQUE, - MessageReceiverError.duplicateMessage, - MessageReceiverError.duplicateControlMessage, - MessageReceiverError.selfSend: - break + messages + .compactMap { message -> ProcessedMessage? in + do { + return try Message.processRawReceivedMessage(db, rawMessage: message) + } + catch { + switch error { + // Ignore duplicate & selfSend message errors (and don't bother logging + // them as there will be a lot since we each service node duplicates messages) + case DatabaseError.SQLITE_CONSTRAINT_UNIQUE, + MessageReceiverError.duplicateMessage, + MessageReceiverError.duplicateControlMessage, + MessageReceiverError.selfSend: + break + + default: SNLog("Failed to deserialize envelope due to error: \(error).") + } - default: SNLog("Failed to deserialize envelope due to error: \(error).") + return nil } } - } - - messageCount = threadMessages - .values - .reduce(into: 0) { prev, next in prev += next.count } - - threadMessages.forEach { threadId, threadMessages in - JobRunner.add( - db, - job: Job( - variant: .messageReceive, - behaviour: .runOnce, - threadId: threadId, - details: MessageReceiveJob.Details( - messages: threadMessages, - isBackgroundPoll: false + .grouped { threadId, _, _ in (threadId ?? Message.nonThreadMessageId) } + .forEach { threadId, threadMessages in + messageCount += threadMessages.count + + JobRunner.add( + db, + job: Job( + variant: .messageReceive, + behaviour: .runOnce, + threadId: threadId, + details: MessageReceiveJob.Details( + messages: threadMessages.map { $0.messageInfo }, + isBackgroundPoll: false + ) ) ) - ) - } + } } SNLog("Received \(messageCount) new message\(messageCount == 1 ? "" : "s") (duplicates: \(messages.count - messageCount))") diff --git a/SessionUtilitiesKit/JobRunner/JobRunner.swift b/SessionUtilitiesKit/JobRunner/JobRunner.swift index 6104b5680..0c1950947 100644 --- a/SessionUtilitiesKit/JobRunner/JobRunner.swift +++ b/SessionUtilitiesKit/JobRunner/JobRunner.swift @@ -688,9 +688,11 @@ private final class JobQueue { } private func scheduleNextSoonestJob() { + let jobIdsAlreadyRunning: Set = jobsCurrentlyRunning.wrappedValue let nextJobTimestamp: TimeInterval? = Storage.shared.read { db in try Job.filterPendingJobs(variants: jobVariants, excludeFutureJobs: false) .select(.nextRunTimestamp) + .filter(!jobIdsAlreadyRunning.contains(Job.Columns.id)) // Exclude jobs already running .asRequest(of: TimeInterval.self) .fetchOne(db) }