centrifugal / centrifuge-swift

Swift client SDK for bidirectional real-time communication with Centrifugo and Centrifuge-based server over WebSocket
MIT License
47 stars 41 forks source link

Make the delegate public so it can be set. #45

Closed onato closed 2 years ago

onato commented 3 years ago

When a subscription already exists I get it with client.getSubscription. The returned subscription's delegate then needs to be set. In order to do this, the delegate needs to be public.

FZambia commented 3 years ago

@onato hello, thanks!

Just want to clarify a use case. So you want to modify delegate behaviour for the existing subscription? Can't you just put the required logic into original delegate implementation passed during Subscription creation time?

onato commented 3 years ago

In my case, not really. I recreate the delegate regularly. This means the delegate is nil when I call getSubscription.

I use the subscription on a detail screen that subscribes to different channels based on its ID. I don't want to keep subscriptions for screens that are no longer around, so I removeSubscription them when I deinit the screen. It shouldn't, given my use-case, but I have seen newSubscription throw a CentrifugeError.duplicateSub. I am using getSubscription to recover. This could be caused by a UISplitViewController keeping a view controller around that is not onscreen. When I reopen that screen I briefly have two screens open that want to be subscribed to the same channel.

I am a little unclear as to how things are intended to be architected so I might be swimming upstream a little. Re-setting the delegate does seem to get things working as I expect.

FZambia commented 3 years ago

I think re-setting delegate without full understanding of the underlying problem isn't a good thing to do. This can cause some scary bugs if not accurately used, and I suppose modifying it should be synchronised with internal code that tries to access delegate methods.

If you call removeSubscription and later newSubscription then I believe library should not throw duplicateSub exceptions since removeSubscription removes Subscription from internal registry – and does this under lock.

So hoping on your help here to find out the reason why this happens in your application, with a simple example that reproduces a problem will be much easier to reason about the right fix 🙏

onato commented 2 years ago

I haven't been able to reliably reproduce my problem. I call client?.removeSubscription(subscription) when the app receives either of…

This should mean the subscription is always removed from the internal registry and it generally works as expected. Occasionally, I am getting duplicateSub errors though, so in the catch block, I'm then getting the existing subscription with getSubscription.

onato commented 2 years ago

I found the issue. I was subscribing on didBecomeActive instead of willEnterForeground which doesn't balance with didBecomeActive