Unleash / unleash-proxy-client-swift

Apache License 2.0
14 stars 20 forks source link

Polling does not work when start called from async context #88

Open benjamin-es-hall opened 4 days ago

benjamin-es-hall commented 4 days ago

Describe the bug

I was trying to wrap calling the start method in modern concurrency so I can call and get completion in async context and I noticed when doing this the poller fails to fire beyond initial fetch.

Steps to reproduce the bug

I've included a simple example below where the start is called on the UnleashClient, and upon starting changes some text in a view to say started or failed to start. For this I added:

  1. A startAsync which wraps the start completion in a checkedContinuation to allow it to be called from async (called from .task view modifier)
  2. A start which wraps the included start method with a completion block (called form .onAppear view modifier)

For both I print out to console ">>> Unleashed!" when it successfully connects. I also have enabled printing status in the client start method.

In both (1) and (2) I successfully see ">>> Unleashed!" print to console showing it did start up. However I only see subsequent client log messages (e.g. No changes in feature toggles.) when calling via the onAppear and completion block.

Seeing how the timer is constructed to fire further requests I see it is added to the RunLoop. I'm thinking that is causing an issue when trying to adapt for modern concurrency.

Any thoughts on a fix? (The poller may need a main dispatch queue added)

import UnleashProxyClientSwift
import SwiftUI

struct ContentView: View {
    @State var text = "Not started"

    var body: some View {
        Text(text)
            .task {
                let started = await Unleasher.shared.startAsync()
                if started {
                    self.text = "Started"
                } else {
                    self.text = "Failed to start"
                }
            }
//            .onAppear {
//                Unleasher.shared.start { started in
//                    if started {
//                        self.text = "Started"
//                   } else {
//                        self.text = "Failed to start"
//                    }
//                }
//            }
    }
}

class Unleasher {
    static let shared = Unleasher()
    private let client: UnleashClient

    private init() {
        self.client = UnleashClient(
            unleashUrl: "someURL",
            clientKey: "someKey",
            refreshInterval: 3,
            appName: "unleasher-ios-sample"
        )
    }

    func startAsync() async -> Bool {
        await withCheckedContinuation { continuation in
            client.start(true) { error in
                if let error {
                    print(error.localizedDescription)
                    continuation.resume(returning: false)
                } else {
                    print(">>> Unleashed!")
                    continuation.resume(returning: true)
                }
            }
        }
    }

    func start(completion: @escaping (Bool) -> Void) {
        client.start(true) { error in
            if let error {
                print(error.localizedDescription)
                completion(false)
            } else {
                print(">>> Unleashed!")
                completion(true)
            }
        }
    }
}

Expected behavior

UnleashClient should continue to fetch even when wrapped in methods porting for calling in modern concurrency

Logs, error output, etc.

No response

Screenshots

No response

Additional context

No response

Unleash version

1.3.0

Subscription type

Open source

Hosting type

Self-hosted

SDK information (language and version)

Only noticed in iOS client SDK

benjamin-es-hall commented 4 days ago

Found this which I think highlights the issue of Timer usage https://wadetregaskis.com/performing-a-delayed-and-or-repeating-operation-in-a-swift-actor/

benjamin-es-hall commented 3 days ago

The example above looks like it has a workaround I'm still exploring but the below is giving me same behaviour as (2)

class Unleasher {
...
    func startAsync() async -> Bool {
        let task = Task { @MainActor in
            await withCheckedContinuation { continuation in
                client.start(true) { error in
                    if let error {
                        print(error.localizedDescription)
                        continuation.resume(returning: false)
                    } else {
                        print(">>> Unleashed!")
                        continuation.resume(returning: true)
                    }
                }
            }
        }

        return await task.value
    }
}
benjamin-es-hall commented 3 days ago

A similar issue is seen in the UnleashClient().updateContext(...) since it calls start within it. The workaround above does not work for this.

FredrikOseberg commented 1 day ago

@benjamin-es-hall

Hey Benjamin. I just wanted to let you know we're looking into this. From what I can tell, it sounds like it makes sense to change the Timer implementation to something that would work concurrently. If you have a solution that can solve the problem and be backwards compatible (also with the supported iOS versions), then I'm happy to review and merge that.

Otherwise we'll need to do some further investigation and prioritise this on our end.

The implementation of the poller is pluggable, so you can provide your own poller implementation that uses a different mechanism than Timer. That could perhaps be a way to go forward?

benjamin-es-hall commented 23 hours ago

Hi @FredrikOseberg ! Thanks for the update. Unfortunately I don't have a solution yet, I've been thinking about it but nothing specific come to mind yet.

I looked into a custom Poller but I think the UnleashClient(Base) takes a concrete Poller in its init rather than an interface/protocol so it's not as simple to override existing behaviour.

FredrikOseberg commented 5 hours ago

Hi @FredrikOseberg ! Thanks for the update. Unfortunately I don't have a solution yet, I've been thinking about it but nothing specific come to mind yet.

I looked into a custom Poller but I think the UnleashClient(Base) takes a concrete Poller in its init rather than an interface/protocol so it's not as simple to override existing behaviour.

Ah yes, you're right. I've put this onto our next sprint, so I'll come back to you when we have a solution.

benjamin-es-hall commented 2 hours ago

If you need me to test anything send me a message and happy to give it a go :)

benjamin-es-hall commented 19 minutes ago

Oh, well I think I found the issue with updateContext and why my workaround for start() was not working there. Turns out updateContext calling start() with its default arguments, which includes setting printToConsole as false, which gives impression it is no longer fetching 🤦

So in all, you can expose start, and update to concurrency as is by doing the following:

class Unleasher {
...
    func startAsync() async -> Bool {
        return Task { @MainActor in
            await withCheckedContinuation { continuation in
                client.start(true) { error in
                    if let error {
                        print(error.localizedDescription)
                        continuation.resume(returning: false)
                    } else {
                        print(">>> Unleashed!")
                        continuation.resume(returning: true)
                    }
                }
            }
        }.value
    }

    func updateContextAsync(context: [String: String], properties: [String: String]? = nil) async -> Bool {
        return Task { @MainActor in
            await withCheckedContinuation { continuation in
                client.updateContext(context: context, properties: properties) { error in
                    if let error {
                        print(error.localizedDescription)
                        continuation.resume(returning: false)
                    } else {
                        print(">>> Context updated!")
                        continuation.resume(returning: true)
                    }
                }
            }
        }.value
    }
}