Reset local device #1253

maratal · 2021-12-12T22:34:10Z

This PR substitutes #1184 (see explanation why in there).
Closes #1177

maratal · 2021-12-12T22:57:20Z

Source/ARTLocalDevice.m

+}
+
+- (BOOL)isReset {
+    return [self.id isEqualToString:@""];


@lawrence-forooghian I know this looks weird, but is a nullable id any better? Also additional flag, as I said earlier, probably also worse than this. Until the example app be able to handle nil device this is the best I could come up with. WDYT?

Maybe using extern NSString * const as a not set value will be more elegant? You can even compare it using == (memory address)

or self.secret == nil WDYT @lukasz-szyszkowski @lawrence-forooghian @ben-xD

for me nil is way better than the empty string

I think my answer to this question probably depends on @maratal's answer to this question.

Source/ARTLocalDevice.m

lukasz-szyszkowski · 2021-12-13T07:51:55Z

Source/ARTLocalDevice.m

+}
+
+- (BOOL)isReset {
+    return [self.id isEqualToString:@""];


Maybe using extern NSString * const as a not set value will be more elegant? You can even compare it using == (memory address)

lawrence-forooghian · 2021-12-20T12:16:59Z

Source/ARTLocalDevice.h

@@ -16,6 +16,8 @@ NS_ASSUME_NONNULL_BEGIN

 - (instancetype)init NS_UNAVAILABLE;

+- (void)reset;


I don't think this should be part of the public interface – let's move it to ARTLocalDevice+Private.h?

lawrence-forooghian · 2021-12-20T13:20:13Z

Source/ARTRest.m

+ This function is needed to return functioning local device, since after `reset` call
+ you will end up with empty string as a device's `id` on the server.
+ */
+- (ARTLocalDevice *)loadDevice {


Since this calls device_noSync, should this also have noSync in its name to indicate that it's the caller's responsibility to ensure that this method is called on the correct queue? (And then make sure that we respect this requirement in the places where we call this method.)

lawrence-forooghian · 2021-12-20T13:28:12Z

Source/ARTRest.m

@@ -742,6 +742,19 @@ - (ARTLocalDevice *)device_nosync {
    return _sharedDevice;
 }

+/*
+ This function is needed to return functioning local device, since after `reset` call


-[ARTRest device] is still part of the public interface of the library, so I think that method needs to always return a "functioning" local device without needing to first call loadDevice (which isn't even a part of the public interface). Could we not move the logic of "if the device has been reset, then create a new one" into -[ARTRest device_noSync]?

I tried that, but it immediately re-creates the device and thus it's never can be in a isReset state which was the whole point of these changes and underlying issue.

I think that the issue being described by #1177 actually consists of two separate things:

When the device deregisters itself for push notifications, it keeps its existing device details (id, clientId, etc), hence violating RSH3g2a.

Once we've fixed point 1, then we should make sure we respect RSH8b:

The LocalDevice id and deviceSecret attributes are generated, and persisted as part of the LocalDevice state, when required by step (RSH3a2b) in the Activation State Machine.

That is, after the device has deregistered itself for push notifications, it should have "empty" (whatever that actually means) id and deviceSecret until the user tries to register the device for push notifications again. This is what @ben-xD showed happening on Android in his video.

So, I think that point 2 above is what you're referring to by "thus it's never can be in a isReset state", but I disagree that it "was the whole point of these changes" – we're also addressing point 1 above.

Furthermore, I think that trying to address point 2 as part of this PR brings additional complication, because we have never respected RSH8b in our codebase. That is, at the moment, we generate the device id and deviceSecret the first time that -[ARTRest device] is called, instead of waiting until the user tries to register the device for push notifications. That's why we're now finding ourselves trying to answer questions like "should a reset device have an id of @""?" If we were already respecting RSH8b, then we'd already have answers to these questions.

So, I think that perhaps respecting RSH8b should be a separate task. And for me (possibly from lack of knowledge of the spec) it raises questions like those you've already identified – if there are API calls that require the device’s ID, which can be called before the push activation process is started, then where are we meant to get the ID from?

Just to clarify to whoever will read this:

RSH3g2a - "Clears all local DeviceDetails." "On event Deregistered".
RSH8b - re-initializing device's fields with the new id and deviceSecret on event CalledActivate.

I think that perhaps respecting RSH8b should be a separate task

Agreed

if there are API calls that require the device’s ID, which can be called before the push activation process is started

I don't think so, it wouldn't make any sense. Or maybe I misunderstood what you've meant here. All functions that compose requests with the device id doesn't do any checks on it though:

NSMutableURLRequest *request = [NSMutableURLRequest requestWithURL:[[NSURL URLWithString:@"/push/deviceRegistrations"] URLByAppendingPathComponent:local.id]];

@lawrence-forooghian #1256

Or maybe I misunderstood what you've meant here.

So, my comment was in response to your code comment on -[ARTRest localDevice]:

This function is needed to return functioning local device, since after reset call
you will end up with empty string as a device's id on the server.

I interpreted that comment as meaning the following:

After calling reset, the localDevice's id becomes an empty string. If we make an API request that uses the device's id whilst in this empty string state, then we'll submit an empty string id to the server, hence we "will end up with empty string as a device's id on the server".

Did I misunderstand?

Did I misunderstand?

No, all is correct, but:

so I think that method (device) needs to always return a "functioning" local device without needing to first call loadDevice

How is it possible to have an empty id then? It's either this or that ("reset" state or always "functioning"). That's why loadDevice exist - to load it on demand (when the function needs it). Anyways lets further discuss it in a call.

if there are API calls that require the device’s ID, which can be called before the push activation process is started

Or maybe you've meant wether id might be required somewhere else outside of the scope of push notifications? Then no, AFAIK.

lawrence-forooghian · 2021-12-20T13:30:36Z

Source/ARTPush+Private.h

@@ -36,4 +36,10 @@ NS_ASSUME_NONNULL_BEGIN

 @end

+@interface NSData (APNSToken)
+
+- (NSString *)apnsTokenString;


Might it be a good idea to prefix this name with art_?

lawrence-forooghian · 2021-12-20T13:34:50Z

Source/ARTRest.m

@@ -788,5 +788,12 @@ - (void)setAndPersistAPNSDeviceToken:(NSString *)deviceToken {
    });
 }

+- (void)resetDevice {
+    dispatch_sync(ARTRestInternal.storageQueue, ^{
+        ARTLocalDevice *device = [self device_nosync];


My understanding of the *_noSync methods is that they need to be manually synchronised with the rest of the library – i.e. they need to be called on the clientOptions’ internalDispatchQueue. Although you're calling device_noSync on a queue here, I don't think that's enough.

Originally the caller in ARTPushActivationStateWaitingForDeregistration did this:

ARTLocalDevice *local = self.machine.rest.device_nosync; [local setAndPersistIdentityTokenDetails:nil];

So I thought that

- (void)resetDevice { dispatch_sync(ARTRestInternal.storageQueue, ^{ ARTLocalDevice *device = [self device]; // <-- ARTRest queue sync call

Would be too much. I'm still not sure about this though.

The difference is that the original ARTPushActivationStateWaitingForDeregistration code
was always called on the ARTRest internal queue and hence could use the *_noSync methods. So I think you need to use the device method here — why do you think it might be "too much"?

storageQueue is a defense against competing ARTRest instances. So I guess I can't use other queue then the storage queue here. And not only this, but having dispatch_once replaced with _sharedDevice static variable, all access to the device should now be done via this storage queue.

lawrence-forooghian · 2021-12-20T13:37:44Z

Source/ARTRest+Private.h

+- (nullable NSString *)apnsDeviceToken;
+- (void)setAndPersistAPNSDeviceToken:(nullable NSString *)deviceToken;
+
+@end


I'm not 100% sure what the purpose of this new, synchronised storage is. But, just going by symmetry, would it not need to also include setAndPersistIdentityTokenDetails:?

what the purpose of this new, synchronised storage is

It was discussed in slack here https://ably-real-time.slack.com/archives/CSQEKCE81/p1638465303293500?thread_ts=1638354865.280100&cid=CSQEKCE81

would it not need to also include setAndPersistIdentityTokenDetails:

Agreed, but it wasn't used anywhere, so I decided to implement only the bare minimum.

Sorry, perhaps I'm not understanding - what do you mean by "it wasn't used anywhere"? setAndPersistIdentityTokenDetails is still called in ARTPushActivationState.m.

It was discussed in slack here

OK, thanks for the link. I think I understand the motivation for the new queue a bit better now, but is it directly relevant to this PR, which is about resetting device details? It seems to me like it might be a separate change.

It seems to me like it might be a separate change.

Please see this comment #1253 (comment)

setAndPersistIdentityTokenDetails is still called in ARTPushActivationState.m

Yes, I should inspect more places where storage queue access should be implemented, thanks.

I've spent a while trying to understand this storage queue better, but it might be best if we have a call to talk about it, to prevent this becoming a long back-and-forth. What do you think?

But, here's where my current thinking is at. I still don't understand why it's necessary to introduce a new queue in order to achieve the overall objective of this pull request. (Whether it's necessary for other reasons is a different matter.)

Looking at what's contained in the ARTRestInternal (Storage) category:

apnsDeviceToken / setAndPersistAPNSDeviceToken – access to the APNS device token was not synchronised in the past, why does the fact that we might now reset the device introduce a need to synchronise it?

resetDevice – this method is already being called on a serial queue because it's only called inside a push activation state machine transition method, so why do we need an extra serial queue?

It might be that, if you agree with my analysis that we don't need to put the device into an intermediate "reset" state, then some of this might simplify when you make that change, too.

then some of this might simplify

Sure, upon splitting this PR into two, this one is going to contain only the very first commit or so.

we don't need to put the device into an intermediate "reset" state

I agreed with the splitting this PR, we still need something that marks the device as "reset", no? Lets discuss it in a call.

maratal · 2022-01-06T15:08:46Z

Closed in favor of #1259

maratal added 6 commits December 12, 2021 21:23

Full reset of the device instead of nullify identity token details

a240961

Tests updated for reset method

c863d01

Global static shared device variable instead of local

2fd8007

Returning functioning local device object after reset was called

1294191

Moved APNS string from NSData creation into category

3db8d95

ARTRestInternal now has static storage dispatch queue.

a414baa

maratal requested review from lawrence-forooghian, ben-xD, QuintinWillison and lukasz-szyszkowski December 12, 2021 22:34

maratal mentioned this pull request Dec 12, 2021

Full reset device details #1184

Closed

maratal commented Dec 12, 2021

View reviewed changes

ARTRestInternal Storage category visibility for iOS only

ae91af8

lukasz-szyszkowski reviewed Dec 13, 2021

View reviewed changes

Resetting device within storage queue

a88c282

lawrence-forooghian reviewed Dec 20, 2021

View reviewed changes

QuintinWillison removed the request for review from ben-xD December 21, 2021 16:38

This was referenced Dec 22, 2021

Respecting RSH8b - generate new local device's id and secret on push activation #1256

Open

Fix/1177 full reset device details (RSH3g2a only) #1259

Merged

maratal closed this Jan 6, 2022

maratal mentioned this pull request Dec 13, 2023

[SDK-1181] Setting clientId again. #1842

Closed

maratal mentioned this pull request Dec 21, 2023

Proper RSH8b and RSH3g2a behavior. #1847

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset local device #1253

Reset local device #1253

maratal commented Dec 12, 2021 •

edited

Loading

maratal Dec 12, 2021

lukasz-szyszkowski Dec 13, 2021

maratal Dec 16, 2021

lukasz-szyszkowski Dec 17, 2021

lawrence-forooghian Dec 20, 2021

lukasz-szyszkowski Dec 13, 2021

lawrence-forooghian Dec 20, 2021

lawrence-forooghian Dec 20, 2021

lawrence-forooghian Dec 20, 2021

maratal Dec 21, 2021

lawrence-forooghian Dec 22, 2021 •

edited

Loading

maratal Dec 22, 2021 •

edited

Loading

maratal Dec 22, 2021

maratal Dec 22, 2021

lawrence-forooghian Dec 23, 2021 •

edited

Loading

maratal Dec 24, 2021

maratal Dec 24, 2021

lawrence-forooghian Dec 20, 2021

lawrence-forooghian Dec 20, 2021

maratal Dec 21, 2021

lawrence-forooghian Dec 22, 2021

maratal Dec 22, 2021

lawrence-forooghian Dec 20, 2021

maratal Dec 21, 2021 •

edited

Loading

lawrence-forooghian Dec 22, 2021

lawrence-forooghian Dec 22, 2021 •

edited

Loading

maratal Dec 22, 2021

maratal Dec 22, 2021

lawrence-forooghian Dec 23, 2021 •

edited

Loading

maratal Dec 23, 2021

maratal commented Jan 6, 2022

		@@ -16,6 +16,8 @@ NS_ASSUME_NONNULL_BEGIN

		- (instancetype)init NS_UNAVAILABLE;

		- (void)reset;

Reset local device #1253

Reset local device #1253

Conversation

maratal commented Dec 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawrence-forooghian Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

maratal Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawrence-forooghian Dec 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maratal Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawrence-forooghian Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawrence-forooghian Dec 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maratal commented Jan 6, 2022

maratal commented Dec 12, 2021 •

edited

Loading

lawrence-forooghian Dec 22, 2021 •

edited

Loading

maratal Dec 22, 2021 •

edited

Loading

lawrence-forooghian Dec 23, 2021 •

edited

Loading

maratal Dec 21, 2021 •

edited

Loading

lawrence-forooghian Dec 22, 2021 •

edited

Loading

lawrence-forooghian Dec 23, 2021 •

edited

Loading