From a563cddba82c07a408c02e13be4ecba025ae499b Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 9 Jan 2024 14:26:09 +1100 Subject: [PATCH] Added a timeout for the MessageSendJob and additional logging --- .../Database/Models/Attachment.swift | 2 +- .../Jobs/Types/MessageSendJob.swift | 15 +++++++++++++-- .../Errors/MessageSenderError.swift | 3 +++ SessionSnodeKit/Networking/OnionRequestAPI.swift | 8 ++++---- 4 files changed, 21 insertions(+), 7 deletions(-) diff --git a/SessionMessagingKit/Database/Models/Attachment.swift b/SessionMessagingKit/Database/Models/Attachment.swift index 91a04b63f..be8186ab1 100644 --- a/SessionMessagingKit/Database/Models/Attachment.swift +++ b/SessionMessagingKit/Database/Models/Attachment.swift @@ -990,7 +990,7 @@ extension Attachment { return downloadUrl .map { urlString -> String? in urlString - .split(separator: "/") + .split(separator: "/") // stringlint:disable .last .map { String($0) } } diff --git a/SessionMessagingKit/Jobs/Types/MessageSendJob.swift b/SessionMessagingKit/Jobs/Types/MessageSendJob.swift index df411c1e0..66da9eca4 100644 --- a/SessionMessagingKit/Jobs/Types/MessageSendJob.swift +++ b/SessionMessagingKit/Jobs/Types/MessageSendJob.swift @@ -163,7 +163,8 @@ public enum MessageSendJob: JobExecutor { // Store the sentTimestamp from the message in case it fails due to a clockOutOfSync error let originalSentTimestamp: UInt64? = details.message.sentTimestamp - /// Perform the actual message sending + /// Perform the actual message sending - this will timeout if the entire process takes longer than `HTTP.defaultTimeout * 2` + /// which can occur if it needs to build a new onion path (which doesn't actually have any limits so can take forever in rare cases) /// /// **Note:** No need to upload attachments as part of this process as the above logic splits that out into it's own job /// so we shouldn't get here until attachments have already been uploaded @@ -183,13 +184,23 @@ public enum MessageSendJob: JobExecutor { .flatMap { MessageSender.sendImmediate(data: $0, using: dependencies) } .subscribe(on: queue, using: dependencies) .receive(on: queue, using: dependencies) + .timeout(.milliseconds(Int(HTTP.defaultTimeout * 2 * 1000)), scheduler: queue, customError: { + MessageSenderError.sendJobTimeout + }) .sinkUntilComplete( receiveCompletion: { result in switch result { case .finished: success(job, false, dependencies) case .failure(let error): - SNLog("[MessageSendJob] Couldn't send message due to error: \(error).") + switch error { + case MessageSenderError.sendJobTimeout: + SNLog("[MessageSendJob] Couldn't send message due to error: \(error) (paths: \(OnionRequestAPI.paths.prettifiedDescription)).") + + default: + SNLog("[MessageSendJob] Couldn't send message due to error: \(error).") + } + // Actual error handling switch error { case let senderError as MessageSenderError where !senderError.isRetryable: failure(job, error, true, dependencies) diff --git a/SessionMessagingKit/Sending & Receiving/Errors/MessageSenderError.swift b/SessionMessagingKit/Sending & Receiving/Errors/MessageSenderError.swift index 5eab36e13..38c499f36 100644 --- a/SessionMessagingKit/Sending & Receiving/Errors/MessageSenderError.swift +++ b/SessionMessagingKit/Sending & Receiving/Errors/MessageSenderError.swift @@ -14,6 +14,7 @@ public enum MessageSenderError: LocalizedError, Equatable { case noUsername case attachmentsNotUploaded case blindingFailed + case sendJobTimeout // Closed groups case noThread @@ -43,6 +44,7 @@ public enum MessageSenderError: LocalizedError, Equatable { case .noUsername: return "Missing username." case .attachmentsNotUploaded: return "Attachments for this message have not been uploaded." case .blindingFailed: return "Couldn't blind the sender" + case .sendJobTimeout: return "Send job timeout (likely due to path building taking too long)." // Closed groups case .noThread: return "Couldn't find a thread associated with the given group public key." @@ -66,6 +68,7 @@ public enum MessageSenderError: LocalizedError, Equatable { case (.noKeyPair, .noKeyPair): return true case (.invalidClosedGroupUpdate, .invalidClosedGroupUpdate): return true case (.blindingFailed, .blindingFailed): return true + case (.sendJobTimeout, .sendJobTimeout): return true case (.other(let lhsError), .other(let rhsError)): // Not ideal but the best we can do diff --git a/SessionSnodeKit/Networking/OnionRequestAPI.swift b/SessionSnodeKit/Networking/OnionRequestAPI.swift index e4e6d505b..ea7fd43f0 100644 --- a/SessionSnodeKit/Networking/OnionRequestAPI.swift +++ b/SessionSnodeKit/Networking/OnionRequestAPI.swift @@ -547,10 +547,10 @@ public enum OnionRequestAPI { switch result { case .finished: break case .failure(let error): - guard - case HTTPError.httpRequestFailed(let statusCode, let data) = error, - let guardSnode: Snode = guardSnode - else { return } + guard let guardSnode: Snode = guardSnode else { + return SNLog("Request failed with no guardSnode.") + } + guard case HTTPError.httpRequestFailed(let statusCode, let data) = error else { return } let path = paths.first { $0.contains(guardSnode) }