[SDK-1181] Setting clientId again.

[maratal]

After clientId has been reset on deactivate, it will be nil during further calls to activate until app relaunch or re-auth to Ably. Fixing it by setting clientId from options again on device activation.

[lawrence-forooghian]

It looks like this PR is implementing RSH8b, which describes how and when the LocalDevice’s clientId should be populated.

Assuming that we're not currently conforming to RSH8b, that presumably means that we must be currently populating LocalDevice.clientId in some non spec-conforming way. Is that right? If so, should we not remove it?

[lawrence-forooghian]

Why the nil check? RSH8b doesn't describe this.

[maratal]

Seems like according to RSH3a2a1 different clientId should have different deviceId and deviceSecret and thus to change clientId full reset device details should be performed. WDYT @lawrence-forooghian

[lawrence-forooghian]

I don't know what you mean by "different clientId should have different deviceId and deviceSecret". What do you mean by this, and how do you take it from reading RSH3a2a1?

[lawrence-forooghian]

I thought that in order to address the issue that you described in the PR description, your PR intended to implement RSH8b properly. Have I missed something?

[maratal]

I don't know what you mean by "different clientId should have different deviceId and deviceSecret". What do you mean by this, and how do you take it from reading RSH3a2a1?

RSH3a2a1: "if the LocalDevice has a non-empty clientId, and the present identified client has a different (non-null) clientId" which I guess explicitly means that if the clientId in the current client (passed through options?) differs from the one saved in the LocalDevice storage (together with id and secret), then it's an error 61002. Which means, that to avoid this error, for a new clientId a new pair of device id and secret is needed.

[lawrence-forooghian]

RSH3a2a1 is executed "if the local device has deviceIdentityToken". But your description for the PR is talking about the user calling activate() having previously deactivated their device, which would have caused any saved deviceIdentityToken to be cleared, so how is RSH3a2a1 relevant to this situation?

[ttypic]

@maratal @lawrence-forooghian I am not sure that the bug is covered by spec. I guess the problem is that spec says:

• LocalDevice is lazy-singleton, it should be initialized on first call
• LocalDevice most likely sets clientId during initialization (and I can't find any spec items that describe when it should be changed)

Customer creates several Ably instances with different clientId during App lifecycle. When they create new Ably instance, with new clientId they get LocalDevice with old clientId and send old clientId everywhere. We tried to fix it by setting LocalDevice.clientId to nil during deregistration, but it made things worse (according to customer, I think it actually makes things a little better) because now customer gets LocalDevice with clientId = nil and get errors even if clientId hasn't been changed.

Does it make sense to you guys?

Can we check clientId handling transition from not_active to the next state?

P.S. I think that LocalDevice is singleton is a bad thing, and it's holding us back

[maratal]

we're not currently conforming to RSH8b

TBH I didn't understand this statement, because what load: does here is initializing clientId while generating device's id and secret a few lines below. So I think that RSH8b is implemented. @lawrence-forooghian

What this PR was initially intended to do, is to fix the first PR that was just resetting clientId which, as @ttypic said, made things looking worse, but in fact, slightly better.

Re RSH3a2a1, there are few tests, that began to fail repeatedly, and I've made erroneous assumption about RSH3a2a1, sorry 🤦‍♂️

Now I think that the best option would be to resurrect the old PR which makes full reset device details and address users complains about it accordingly.

[maratal]

Ok, now I see what you've meant here re RSH8b @lawrence-forooghian

[lawrence-forooghian]

• LocalDevice most likely sets clientId during initialization (and I can't find any spec items that describe when it should be changed)

I think that my old message that Marat linked to above does the best job of explaining this.

RSH8b describes when the LocalDevice’s clientId should be set. That is, it should not be set when the singleton is lazily instantiated; it should be set at a specific point in the push activation flow. This behaviour is currently missing in ably-cocoa, and is the reason why the clientId is currently not being set again after the device is re-activated.

We tried to fix it by setting LocalDevice.clientId to nil during deregistration

Yes, this was us implementing RSH3g2a, right? But that spec point needs to be implemented alongside RSH8b.

As far as I can tell, as long as we're properly implementing RSH8b and RSH3g2a, we shouldn't have any problems. Have I missed something?

[maratal]

As far as I can tell, as long as we're properly implementing RSH8b and RSH3g2a, we shouldn't have any problems. Have I missed something?

I think that those specs should be implemented in different PRs, but shouldn't be shipped separately (release should contain them both).

[lawrence-forooghian]

Sure, we can fix them however we want. But do you agree that all we need to do is make sure we're correctly implementing those two spec points and this whole issue should go away?

[lawrence-forooghian]

P.S. I think that LocalDevice is singleton is a bad thing, and it's holding us back

Yes, agreed, but I think that as long as the user is properly deactivating one clientId before starting to use a different one, then it's OK right?

[maratal]

Sure, we can fix them however we want. But do you agree that all we need to do is make sure we're correctly implementing those two spec points and this whole issue should go away?

Well, at least it looks like that.

[maratal]

Closed in favor of #1847