Reset local device #1253
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,4 +110,23 @@ - (BOOL)isRegistered { | |
| return _identityTokenDetails != nil; | ||
| } | ||
|
|
||
| - (void)clearStorage { | ||
lukasz-szyszkowski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| for (NSString *key in @[ ARTDeviceIdKey, ARTDeviceIdentityTokenKey ]) { | ||
| [self.storage setObject:nil forKey:key]; | ||
| } | ||
| } | ||
|
|
||
| - (void)reset { | ||
| self.id = @""; | ||
| self.secret = nil; | ||
| self.clientId = nil; | ||
| _identityTokenDetails = nil; | ||
| [self.push.recipient removeAllObjects]; | ||
| [self clearStorage]; | ||
| } | ||
|
|
||
| - (BOOL)isReset { | ||
| return [self.id isEqualToString:@""]; | ||
|
There was a problem hiding this comment. @lawrence-forooghian I know this looks weird, but is a There was a problem hiding this comment. Maybe using There was a problem hiding this comment. or There was a problem hiding this comment. for me There was a problem hiding this comment. I think my answer to this question probably depends on @maratal's answer to this question. |
||
| } | ||
|
|
||
| @end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,6 +65,7 @@ NS_ASSUME_NONNULL_BEGIN | |
|
|
||
| #if TARGET_OS_IOS | ||
| - (void)resetDeviceSingleton; | ||
| - (ARTLocalDevice *)loadDevice; | ||
| #endif | ||
|
|
||
| @end | ||
|
|
@@ -77,4 +78,16 @@ NS_ASSUME_NONNULL_BEGIN | |
|
|
||
| @end | ||
|
|
||
| #if TARGET_OS_IOS | ||
| @interface ARTRestInternal (Storage) | ||
|
|
||
| + (dispatch_queue_t)storageQueue; | ||
|
|
||
| - (nullable NSString *)apnsDeviceToken; | ||
| - (void)setAndPersistAPNSDeviceToken:(nullable NSString *)deviceToken; | ||
| - (void)resetDevice; | ||
|
|
||
| @end | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment.
It was discussed in slack here https://ably-real-time.slack.com/archives/CSQEKCE81/p1638465303293500?thread_ts=1638354865.280100&cid=CSQEKCE81
Agreed, but it wasn't used anywhere, so I decided to implement only the bare minimum. There was a problem hiding this comment. Sorry, perhaps I'm not understanding - what do you mean by "it wasn't used anywhere"? There was a problem hiding this comment.
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. There was a problem hiding this comment.
Please see this comment #1253 (comment) There was a problem hiding this comment.
Yes, I should inspect more places where storage queue access should be implemented, thanks. There was a problem hiding this comment. 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
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. There was a problem hiding this comment.
Sure, upon splitting this PR into two, this one is going to contain only the very first commit or so.
I agreed with the splitting this PR, we still need something that marks the device as "reset", no? Lets discuss it in a call. |
||
| #endif | ||
|
|
||
| NS_ASSUME_NONNULL_END | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -716,6 +716,9 @@ - (void)setCurrentFallbackHost:(NSString *)value { | |
| } | ||
|
|
||
| #if TARGET_OS_IOS | ||
|
|
||
| static ARTLocalDevice *_sharedDevice; | ||
|
|
||
| - (ARTLocalDevice *)device { | ||
| __block ARTLocalDevice *ret; | ||
| dispatch_sync(_queue, ^{ | ||
|
|
@@ -724,9 +727,6 @@ - (ARTLocalDevice *)device { | |
| return ret; | ||
| } | ||
|
|
||
| - (ARTLocalDevice *)device_nosync { | ||
| // The device is shared in a static variable because it's a reflection | ||
| // of what's persisted. Having a device instance per ARTRest instance | ||
|
|
@@ -736,18 +736,64 @@ - (ARTLocalDevice *)device_nosync { | |
| // As a side effect, the first instance "wins" at setting the device's | ||
| // client ID. | ||
|
|
||
| if (_sharedDevice == nil) { | ||
| _sharedDevice = [ARTLocalDevice load:self.auth.clientId_nosync storage:self.storage]; | ||
| } | ||
| return _sharedDevice; | ||
| } | ||
|
|
||
| /* | ||
| This function is needed to return functioning local device, since after `reset` call | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. I tried that, but it immediately re-creates the device and thus it's never can be in a There was a problem hiding this comment. I think that the issue being described by #1177 actually consists of two separate things:
So, I think that point 2 above is what you're referring to by "thus it's never can be in a 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 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? There was a problem hiding this comment.
Agreed
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
There was a problem hiding this comment. There was a problem hiding this comment.
So, my comment was in response to your code comment on
I interpreted that comment as meaning the following: After calling Did I misunderstand? There was a problem hiding this comment.
No, all is correct, but:
How is it possible to have an empty There was a problem hiding this comment.
Or maybe you've meant wether |
||
| you will end up with empty string as a device's `id` on the server. | ||
| */ | ||
| - (ARTLocalDevice *)loadDevice { | ||
|
There was a problem hiding this comment. Since this calls |
||
| ARTLocalDevice *device = [self device_nosync]; | ||
| if (device.isReset) { | ||
| [self resetDeviceSingleton]; | ||
| device = [self device_nosync]; | ||
| } | ||
| return device; | ||
| } | ||
|
|
||
| - (void)resetDeviceSingleton { | ||
| _sharedDevice = nil; | ||
| } | ||
| #endif | ||
|
|
||
| @end | ||
|
|
||
| #if TARGET_OS_IOS | ||
| @implementation ARTRestInternal (Storage) | ||
|
|
||
| + (dispatch_queue_t)storageQueue { | ||
| static dispatch_queue_t queue; | ||
| static dispatch_once_t onceToken; | ||
| dispatch_once(&onceToken, ^{ | ||
| queue = dispatch_queue_create("io.ably.device-storage", DISPATCH_QUEUE_SERIAL); | ||
| }); | ||
| return queue; | ||
| } | ||
|
|
||
| - (NSString *)apnsDeviceToken { | ||
| __block NSString *value; | ||
| dispatch_sync(ARTRestInternal.storageQueue, ^{ | ||
| value = [[self device_nosync] apnsDeviceToken]; | ||
| }); | ||
| return value; | ||
| } | ||
|
|
||
| - (void)setAndPersistAPNSDeviceToken:(NSString *)deviceToken { | ||
| dispatch_sync(ARTRestInternal.storageQueue, ^{ | ||
| [[self device_nosync] setAndPersistAPNSDeviceToken:deviceToken]; | ||
| }); | ||
| } | ||
|
|
||
| - (void)resetDevice { | ||
| dispatch_sync(ARTRestInternal.storageQueue, ^{ | ||
| ARTLocalDevice *device = [self device_nosync]; | ||
|
There was a problem hiding this comment. My understanding of the There was a problem hiding this comment. Originally the caller in So I thought that Would be too much. I'm still not sure about this though. There was a problem hiding this comment. The difference is that the original There was a problem hiding this comment.
|
||
| [device reset]; | ||
| }); | ||
| } | ||
|
|
||
| @end | ||
| #endif | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be part of the public interface – let's move it to
ARTLocalDevice+Private.h?