Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reset local device #1253

Closed
wants to merge 8 commits into from
Closed

Reset local device #1253

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a240961
Full reset of the device instead of nullify identity token details
maratal Dec 9, 2021
c863d01
Tests updated for `reset` method
maratal Dec 9, 2021
2fd8007
Global static shared device variable instead of local
maratal Dec 12, 2021
1294191
Returning functioning local device object after `reset` was called
maratal Dec 11, 2021
3db8d95
Moved APNS string from NSData creation into category
maratal Dec 10, 2021
a414baa
ARTRestInternal now has static storage dispatch queue.
maratal Dec 12, 2021
ae91af8
ARTRestInternal Storage category visibility for iOS only
maratal Dec 12, 2021
a88c282
Resetting device within storage queue
maratal Dec 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Source/ARTPushActivationState.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,7 @@ - (ARTPushActivationState *)transition:(ARTPushActivationEvent *)event {
}
else if ([event isKindOfClass:[ARTPushActivationEventDeregistered class]]) {
#if TARGET_OS_IOS
ARTLocalDevice *device = self.machine.rest.loadDevice;
[device reset];
[self.machine.rest resetDevice];
#endif
[self.machine callDeactivatedCallback:nil];
return [ARTPushActivationStateNotActivated newWithMachine:self.machine];
Expand Down
1 change: 1 addition & 0 deletions Source/ARTRest+Private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ NS_ASSUME_NONNULL_BEGIN

- (nullable NSString *)apnsDeviceToken;
- (void)setAndPersistAPNSDeviceToken:(nullable NSString *)deviceToken;
- (void)resetDevice;

@end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:?

Copy link
Collaborator Author

@maratal maratal Dec 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian Dec 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Please see this comment #1253 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setAndPersistIdentityTokenDetails is still called in ARTPushActivationState.m

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

Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian Dec 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#endif
Expand Down
7 changes: 7 additions & 0 deletions Source/ARTRest.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -788,5 +788,12 @@ - (void)setAndPersistAPNSDeviceToken:(NSString *)deviceToken {
});
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

[device reset];
});
}

@end
#endif