Skip to content

Commit

Permalink
Possibly fix phantom local push subscription not alerting (#1800)
Browse files Browse the repository at this point in the history
Refs #1799.

## Summary
I can't see too many cases where we'd drop a subscription, but this is one of them - basically, multiple connection info change notifications fire at once, and the invoked method isn't threadsafe.

## Any other notes
Also adds some more logging and discrete subscription cancelling when stopping to hopefully help debug this in the future if it continues.
  • Loading branch information
zacwest authored Jul 19, 2021
1 parent d5a3c70 commit dcc15a1
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 25 deletions.
1 change: 1 addition & 0 deletions Sources/Extensions/PushProvider/PushProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ import UserNotifications

override func stop(with reason: NEProviderStopReason, completionHandler: @escaping () -> Void) {
Current.Log.notify("stopping with reason \(reason)", log: .error)
localPushManager?.invalidate()
localPushManager = nil
Current.apiConnection.disconnect()
}
Expand Down
27 changes: 22 additions & 5 deletions Sources/Shared/Notifications/LocalPush/LocalPushManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -80,18 +80,28 @@ public class LocalPushManager {
}

public init() {
updateSubscription()

NotificationCenter.default.addObserver(
self,
selector: #selector(updateSubscription),
selector: #selector(updateSubscriptionFromNotification),
name: SettingsStore.connectionInfoDidChange,
object: nil
)

updateSubscription()
}

deinit {
subscription?.cancel()
invalidate()
}

public func invalidate() {
if let subscription = subscription {
Current.Log.info("cancelling")
subscription.cancel()
} else {
Current.Log.info("no active subscription")
}
NotificationCenter.default.removeObserver(self)
}

var add: (UNNotificationRequest) -> Promise<Void> = { request in
Expand All @@ -105,13 +115,20 @@ public class LocalPushManager {
let webhookID: String

func cancel() {
Current.Log.info("cancelling subscription")
token.cancel()
}
}

private var subscription: SubscriptionInstance?

@objc private func updateSubscription() {
@objc private func updateSubscriptionFromNotification() {
DispatchQueue.main.async { [self] in
updateSubscription()
}
}

private func updateSubscription() {
guard let webhookID = Current.settingsStore.connectionInfo?.webhookID else {
// webhook is invalid, if there is a subscription we remove it
subscription?.cancel()
Expand Down
48 changes: 28 additions & 20 deletions Tests/Shared/LocalPushManager.test.swift
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ class LocalPushManagerTests: XCTestCase {
weak var weakManager = manager
manager = nil
XCTAssertNil(weakManager)

for sub in apiConnection.pendingSubscriptions {
XCTAssertTrue(sub.cancellable.wasCancelled)
}
}

private func setUpManager(webhookID: String?) {
Expand Down Expand Up @@ -67,6 +71,19 @@ class LocalPushManagerTests: XCTestCase {
}
}

private func fireConnectionChange() {
NotificationCenter.default.post(
name: SettingsStore.connectionInfoDidChange,
object: nil,
userInfo: nil
)
let expectation = self.expectation(description: "loop")
DispatchQueue.main.async {
expectation.fulfill()
}
wait(for: [expectation], timeout: 10)
}

func testStateInitialUnavailable() {
setUpManager(webhookID: nil)
XCTAssertEqual(manager.state, .unavailable)
Expand Down Expand Up @@ -119,20 +136,12 @@ class LocalPushManagerTests: XCTestCase {
sub1.initiated(.success(.empty))

apiConnection.pendingSubscriptions.removeAll()
NotificationCenter.default.post(
name: SettingsStore.connectionInfoDidChange,
object: nil,
userInfo: nil
)
fireConnectionChange()
XCTAssertTrue(apiConnection.pendingSubscriptions.isEmpty, "same id")

// change webhookID
Current.settingsStore.connectionInfo?.webhookID = "webhook2"
NotificationCenter.default.post(
name: SettingsStore.connectionInfoDidChange,
object: nil,
userInfo: nil
)
fireConnectionChange()

XCTAssertTrue(sub1.cancellable.wasCancelled)

Expand All @@ -142,27 +151,26 @@ class LocalPushManagerTests: XCTestCase {

// fail the subscription
sub2.initiated(.failure(.internal(debugDescription: "unit-test")))
NotificationCenter.default.post(
name: SettingsStore.connectionInfoDidChange,
object: nil,
userInfo: nil
)
fireConnectionChange()

// now succeed it (e.g. reconnect happened)
sub2.initiated(.success(.empty))

// kill off the connection info
apiConnection.pendingSubscriptions.removeAll()
Current.settingsStore.connectionInfo = nil
NotificationCenter.default.post(
name: SettingsStore.connectionInfoDidChange,
object: nil,
userInfo: nil
)
fireConnectionChange()

XCTAssertTrue(sub2.cancellable.wasCancelled)
}

func testInvalidate() throws {
setUpManager(webhookID: "webhook1")
let sub = try XCTUnwrap(apiConnection.pendingSubscriptions.first)
manager.invalidate()
XCTAssertTrue(sub.cancellable.wasCancelled)
}

func testNoSubscriptionAtStart() throws {
setUpManager(webhookID: nil)
XCTAssertTrue(apiConnection.pendingSubscriptions.isEmpty)
Expand Down

0 comments on commit dcc15a1

Please sign in to comment.