search

ably / ably-cocoa

iOS, tvOS and macOS Objective-C and Swift client library SDK for Ably realtime messaging service
https://ably.com/download
Apache License 2.0
46 stars 25 forks source link

Implement RTP18a #1904

Closed maratal closed 6 months ago

maratal commented 7 months ago

RTP18a: The client library determines that a new sync has started whenever a SYNC ProtocolMessage is received with a channel attribute and a new sync sequence identifier in the channelSerial attribute. The channelSerial is used as the sync cursor and is a two-part identifier :. If a new sequence identifier is sent from Ably, then the client library must consider that to be the start of a new sync sequence and any previous in-flight sync should be discarded

Now library doesn't support this behavior (in-flight sync will not be discarded, instread new sync will be ignored).

This issue is a part of a bigger work (https://github.com/ably/ably-cocoa/pull/1891).

┆Issue is synchronized with this Jira Task by Unito

lawrence-forooghian commented 6 months ago

I’m setting this as a medium priority but it’s a bit of a guess — I don't know how often the described scenario occurs in reality.

sacOO7 commented 6 months ago

I’m setting this as a medium priority but it’s a bit of a guess — I don't know how often the described scenario occurs in reality.

I agree! All other libraries are working fine without implementing this. Even if ably-js supports this spec, being single threaded, there's no issue of synchronization or multithreading unlike other languages.

maratal commented 6 months ago

I’m setting this as a medium priority but it’s a bit of a guess — I don't know how often the described scenario occurs in reality.

I agree! All other libraries are working fine without implementing this. Even if ably-js supports this spec, being single threaded, there's no issue of synchronization or multithreading unlike other languages.

There are no threading issue in this PR though (all happens in the single realtime background thread). Should I close it now or just leave it as-is?

lawrence-forooghian commented 6 months ago

It seems like something we should implement; why would we close it?

maratal commented 6 months ago

It seems like something we should implement; why would we close it?

Sorry, I've meant the PR, not clear to me what to do with it now. Shouldn't be permanently opened, right? If you can help in reviewing it I would appreciate.

sacOO7 commented 6 months ago

@maratal few points to mention

  1. Even though this spec is not implemented in other SDKs, we haven't faced any issues due to lack of it.
  2. Spec is a bit complicated and involves a very rare scenario that's unlikely to happen.
  3. If we do not implement this spec correctly, I'm afraid, it will break existing spec instead of improving it.

Because of this, I think we should hold on to merging it into the main. Maybe in the near future, we can think about merging it. Let's see the feedback from customers about recently merged PR's first 👍