Skip to content

Commit 0369bbd

Browse files
netbenetbe
committed
code review
1 parent 6ee1c2a commit 0369bbd

File tree

7 files changed

+201
-58
lines changed

7 files changed

+201
-58
lines changed

‎WireFoundation/Sources/WireFoundation/Utilities/BackoffRetry/BackoffRetrier.swift‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,14 +36,16 @@ public actor BackoffRetrier {
3636

3737
public init(
3838
policy: BackoffRetryPolicy = .init(),
39+
monitoringNetwork: Bool = true,
3940
sleep: @escaping SleepFunction = { delay in
4041
try await Task.sleep(nanoseconds: UInt64(delay * 1_000_000_000))
4142
}
4243
) {
4344
self.policy = policy
4445
self.sleep = sleep
45-
46-
setupObservers()
46+
if monitoringNetwork {
47+
setupObservers()
48+
}
4749
}
4850

4951
deinit {

‎wire-ios-data-model/Source/MLS/MLSActionExecutor.swift‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -257,7 +257,8 @@ public actor MLSActionExecutor: MLSActionExecutorProtocol {
257257
} catch {
258258
WireLogger.mls
259259
.info(
260-
"failed: adding members to group: \(String(describing: error))", attributes: groupID.safeAttributes
260+
"failed: adding members to group: \(String(describing: error))",
261+
attributes: groupID.safeAttributes
261262
)
262263
throw error
263264
}
@@ -277,7 +278,8 @@ public actor MLSActionExecutor: MLSActionExecutorProtocol {
277278
} catch {
278279
WireLogger.mls
279280
.info(
280-
"error: removing clients from group: \(String(describing: error))", attributes: groupID.safeAttributes
281+
"error: removing clients from group: \(String(describing: error))",
282+
attributes: groupID.safeAttributes
281283
)
282284
throw error
283285
}
@@ -294,7 +296,8 @@ public actor MLSActionExecutor: MLSActionExecutorProtocol {
294296
} catch {
295297
WireLogger.mls
296298
.info(
297-
"error: updating key material for group: \(String(describing: error))", attributes: groupID.safeAttributes
299+
"error: updating key material for group: \(String(describing: error))",
300+
attributes: groupID.safeAttributes
298301
)
299302
throw error
300303
}
@@ -313,7 +316,8 @@ public actor MLSActionExecutor: MLSActionExecutorProtocol {
313316
} catch {
314317
WireLogger.mls
315318
.info(
316-
"error: committing pending proposals for group: \(String(describing: error))", attributes: groupID.safeAttributes
319+
"error: committing pending proposals for group: \(String(describing: error))",
320+
attributes: groupID.safeAttributes
317321
)
318322
throw error
319323
}
@@ -342,7 +346,8 @@ public actor MLSActionExecutor: MLSActionExecutorProtocol {
342346
} catch {
343347
WireLogger.mls
344348
.info(
345-
"error: joining group via external commit: \(String(describing: error))", attributes: groupID.safeAttributes
349+
"error: joining group via external commit: \(String(describing: error))",
350+
attributes: groupID.safeAttributes
346351
)
347352
throw error
348353
}
@@ -421,6 +426,6 @@ extension MLSActionExecutor.Action: CustomDebugStringConvertible {
421426

422427
private extension MLSGroupID {
423428
var safeAttributes: LogAttributes {
424-
[.mlsGroupID: self.safeForLoggingDescription, .public: true]
429+
[.mlsGroupID: safeForLoggingDescription, .public: true]
425430
}
426431
}

‎wire-ios-data-model/Source/MLS/MLSService.swift‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1401,8 +1401,16 @@ public final class MLSService: MLSServiceInterface {
14011401
// MARK: - Pending proposals
14021402

14031403
public func commitPendingProposals(in groupID: MLSGroupID) async throws {
1404-
try await retryOnCommitFailure(for: groupID) { [weak self] in
1405-
try await self?.internalCommitPendingProposals(in: groupID)
1404+
try await commitPendingProposals(in: groupID, skipRetry: false)
1405+
}
1406+
1407+
public func commitPendingProposals(in groupID: MLSGroupID, skipRetry: Bool) async throws {
1408+
if skipRetry {
1409+
try await internalCommitPendingProposals(in: groupID)
1410+
} else {
1411+
try await retryOnCommitFailure(for: groupID) { [weak self] in
1412+
try await self?.internalCommitPendingProposals(in: groupID)
1413+
}
14061414
}
14071415
}
14081416

@@ -1506,7 +1514,7 @@ public final class MLSService: MLSServiceInterface {
15061514
switch RecoveryStrategy(from: reason) {
15071515
case .retryAfterBackoff:
15081516

1509-
try await BackoffRetrier(policy: .init(maxRetries: 2)).retry { [logger] in
1517+
try await BackoffRetrier(policy: .init(maxRetries: 2), monitoringNetwork: false).retry { [logger] in
15101518
logger.warn(
15111519
"failed to send commit due to \(reason). retrying operation with backoff - attempt: \(retryCount)...",
15121520
attributes: logAttributes

‎wire-ios-data-model/Source/MLS/MLSServiceInterface.swift‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,14 @@ public protocol MLSServiceInterface: MLSEncryptionServiceInterface, MLSDecryptio
332332

333333
func commitPendingProposals(in groupID: MLSGroupID) async throws
334334

335+
/// Commits pending proposals for a group.
336+
/// - Parameters:
337+
/// - groupID: The group ID to commit pending proposals for
338+
/// - skipRetry: true to skip internal recovery strategy. False otherwise
339+
///
340+
/// If skipRetry is false it's up to the caller to handle errors
341+
func commitPendingProposals(in groupID: MLSGroupID, skipRetry: Bool) async throws
342+
335343
// MARK: - Key material
336344

337345
/// Updates the key material for all stale groups if it hasn't been done in the past day.

‎wire-ios-data-model/Support/Sourcery/generated/AutoMockable.generated.swift‎

Lines changed: 20 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎wire-ios-request-strategy/Sources/Message Sending/MessageSender.swift‎

Lines changed: 100 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
// along with this program. If not, see http://www.gnu.org/licenses/.
1717
//
1818

19+
import WireCoreCrypto
1920
import WireDataModel
2021
import WireLogging
2122

@@ -398,7 +399,7 @@ public final class MessageSender: MessageSenderInterface {
398399
try await mlsService.reEstablishPendingGroup(groupID: groupID)
399400
}
400401

401-
try await mlsService.commitPendingProposals(in: groupID)
402+
try await mlsService.commitPendingProposals(in: groupID, skipRetry: true)
402403
let encryptedData = try await encryptMlsMessage(message, groupID: groupID)
403404

404405
// set expiration so request can be expired later
@@ -423,51 +424,84 @@ public final class MessageSender: MessageSenderInterface {
423424
message.delivered(with: response)
424425
}
425426
} catch let error as SendMLSMessageFailure {
426-
switch error {
427-
case .mlsStaleMessage:
428-
// We should try to repair the conversation for the `mlsStaleMessage` error.
429-
// This error indicates that the message was not encrypted in the latest epoch.
430-
let operation: () async throws -> Void = { [weak self] in
431-
try await self?.sendMessage(message: message)
432-
}
433427

434-
try await handleMLSStaleMessageError(
428+
try await handleSendMLSMessageFailure(error, message: message, groupID: groupID, mlsService: mlsService)
429+
} catch let CoreCryptoError.Mls(.MessageRejected(reason: reason)) {
430+
431+
if let supportedError = SendMLSMessageFailure(from: reason) {
432+
try await handleSendMLSMessageFailure(
433+
supportedError,
434+
message: message,
435435
groupID: groupID,
436-
mlsService: mlsService,
437-
operation: operation
436+
mlsService: mlsService
438437
)
439-
case .mlsInvalidLeafNodeIndex, .mlsInvalidLeafNodeSignature:
440-
let feature = await featureRepository.fetchAllowedGlobalOperations()
441-
guard feature.status == .enabled,
442-
feature.config.mlsConversationReset == true
443-
else {
444-
WireLogger.messaging.debug(
445-
"No need to initiate reset broken MLS conversation, FF is OFF"
446-
)
447-
throw error
448-
}
438+
} else {
439+
throw CoreCryptoError.Mls(.MessageRejected(reason: reason))
440+
}
449441

450-
let epoch = await context.perform { message.conversation?.epoch }
442+
}
443+
}
451444

452-
await initiateResetMLSConversationUseCase
453-
.invoke(
454-
groupID: groupID,
455-
epoch: epoch ?? 0
456-
)
457-
case let .groupOutOfSync(missingUsers):
458-
guard retryCount < maxRetryAttempts else {
459-
retryCount = 0
460-
throw error
461-
}
445+
private func handleSendMLSMessageFailure(
446+
_ error: SendMLSMessageFailure,
447+
message: any SendableMessage,
448+
groupID: MLSGroupID,
449+
mlsService: MLSServiceInterface
450+
) async throws {
451+
switch error {
452+
case .mlsStaleMessage:
453+
// We should try to repair the conversation for the `mlsStaleMessage` error.
454+
// This error indicates that the message was not encrypted in the latest epoch.
455+
let operation: () async throws -> Void = { [weak self] in
456+
try await self?.sendMessage(message: message)
457+
}
462458

463-
retryCount += 1
459+
try await handleMLSStaleMessageError(
460+
groupID: groupID,
461+
mlsService: mlsService,
462+
operation: operation
463+
)
464+
case .mlsInvalidLeafNodeIndex, .mlsInvalidLeafNodeSignature:
465+
let feature = await featureRepository.fetchAllowedGlobalOperations()
466+
guard feature.status == .enabled,
467+
feature.config.mlsConversationReset == true
468+
else {
469+
WireLogger.messaging.debug(
470+
"No need to initiate reset broken MLS conversation, FF is OFF"
471+
)
472+
throw error
473+
}
474+
475+
let epoch = await context.perform { message.conversation?.epoch }
464476

465-
let users = missingUsers.map { MLSUser($0) }
466-
try await mlsService.addMembersToConversation(with: users, for: groupID)
467-
try await sendMessage(message: message)
468-
default:
477+
await initiateResetMLSConversationUseCase
478+
.invoke(
479+
groupID: groupID,
480+
epoch: epoch ?? 0
481+
)
482+
case let .groupOutOfSync(missingUsers):
483+
guard retryCount < maxRetryAttempts else {
484+
retryCount = 0
469485
throw error
470486
}
487+
488+
retryCount += 1
489+
490+
let users = missingUsers.map { MLSUser($0) }
491+
try await mlsService.addMembersToConversation(with: users, for: groupID)
492+
try await sendMessage(message: message)
493+
case .mlsCommitMissingReferences, .mlsClientMismatch:
494+
// here a simple retry is used but as an optim we could use a backoff
495+
guard retryCount < maxRetryAttempts else {
496+
retryCount = 0
497+
throw error
498+
}
499+
500+
retryCount += 1
501+
502+
try await sendMessage(message: message)
503+
default:
504+
throw error
471505
}
472506
}
473507

@@ -532,3 +566,32 @@ private extension Payload.ClientListByQualifiedUserID {
532566
}
533567

534568
}
569+
570+
private extension SendMLSMessageFailure {
571+
572+
init?(from reason: String) {
573+
guard let error = try? JSONDecoder().decode(
574+
MLSTransportError.self,
575+
from: Data(reason.utf8)
576+
) else {
577+
return nil
578+
}
579+
580+
switch error {
581+
case .mlsClientMismatch:
582+
self = .mlsClientMismatch
583+
case .mlsCommitMissingReferences:
584+
self = .mlsCommitMissingReferences(message: "")
585+
case .mlsStaleMessage:
586+
self = .mlsStaleMessage
587+
case .mlsInvalidLeafNodeIndex:
588+
self = .mlsInvalidLeafNodeIndex(message: "")
589+
case .mlsInvalidLeafNodeSignature:
590+
self = .mlsInvalidLeafNodeSignature(message: "")
591+
case let .groupOutOfSync(missingUsers):
592+
self = .groupOutOfSync(missingUsers: missingUsers)
593+
default:
594+
return nil
595+
}
596+
}
597+
}

0 commit comments

Comments
 (0)