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.

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

Fix registerRemoteNotifications action would never return #115

Merged
merged 9 commits into from
Aug 26, 2024

Conversation

Supereg
Copy link
Member

@Supereg Supereg commented Aug 20, 2024

Fix registerRemoteNotifications action would never return

♻️ Current situation & Problem

#98 introduced the registerRemoteNotifications action. However, the knowledge source that stores the continuation was never persisted in the store. Therefore, the continuation was never resumed and allocated resources forever.
This was fixed with this PR. We originally discussed supporting cancellation of the registration processed, however I decided against as it wasn't clear to properly handle cancellation (e.g., automatically call the unregister action? How to handle the delegate call that might arrive later. Especially when another call to the action might incur in the mean time).
This PR changes the behavior when concurrently calling the registerRemoteNotifications action. Now, the second caller will wait for the first call to complete instead of throwing a concurrence access error. This helps to share this resources with, e.g., multiple modules trying to retrieve an device token.

To emphasize: registerRemoteNotifications works on simulator devices!

⚙️ Release Notes

  • Fixed an issue where the registerRemoteNotifications actions would never return.

📚 Documentation

Slightly updated docs to provide more guidance around requesting authorization for notifications.

✅ Testing

Added a unit test that verifies that the action returns.

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

@Supereg
Copy link
Member Author

Supereg commented Aug 20, 2024

CC: @nriedman

@PSchmiedmayer PSchmiedmayer added the enhancement New feature or request label Aug 20, 2024
Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for fixing the issue and adding all the documentation @Supereg! 🚀

There is an error message due to Xcode project version updated beyond the latest stable Xcode release. I tagged this in the diff.

The main UI tests seem to fail on the simulator; I could not reproduce it locally. Might be an issue with our runners?

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 94.87179% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.60%. Comparing base (7755013) to head (90c7991).
Report is 1 commits behind head on main.

Files Patch % Lines
...ifications/RegisterRemoteNotificationsAction.swift 94.88% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #115      +/-   ##
==========================================
+ Coverage   90.33%   90.60%   +0.27%     
==========================================
  Files          47       47              
  Lines        1602     1626      +24     
==========================================
+ Hits         1447     1473      +26     
+ Misses        155      153       -2     
Files Coverage Δ
...ications/UnregisterRemoteNotificationsAction.swift 100.00% <ø> (ø)
Sources/Spezi/Spezi/Spezi.swift 98.88% <ø> (ø)
...ifications/RegisterRemoteNotificationsAction.swift 92.96% <94.88%> (+7.86%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7755013...90c7991. Read the comment docs.

@Supereg
Copy link
Member Author

Supereg commented Aug 21, 2024

@PSchmiedmayer Executing the UI tests on my local simulator, registerRemoteNotifications always returns a simulated device token (on iOS 17.5 simulators its 80 bytes, while 60 bytes on the iOS 18 simulator). However, this behavior is not reproducible on the CI. There, the call never returns (just as we had before). I couldn't find anything that would document this behavior. It might be that the behavior is influenced by having a local developer account associated with Xcode (maybe you could verify the behavior on your end?).
I'm not quite sure what behavior to document now. My current approach adds a timeout for simulator devices and throws a TimeoutError if the delegate is never called. We could also aim for making the behavior consistent and simulate a token ourselves (in any case the token returned won't be valid)?

@nriedman Would you mind, trying this branch and running the TestApp of the UITests, navigating to Remote Notifications menu and clicking on register. Does the call return for you?

@PSchmiedmayer
Copy link
Member

What a strange behavior. I could imagine this has something to do with the missing developer account or the fact that the build agents run in a virtual machine. Elements like Xcode Apple Intelligence Code suggestions and other elements are also disables when Xcode runs in a VM (I suspect Apple doesn't want to make VMs fully operational to avoid an easy way to virtualize macOS for commercial purposes). That would be my educated guess, I can try to verify this tomorrow.

@PSchmiedmayer
Copy link
Member

PSchmiedmayer commented Aug 21, 2024

@Supereg I can verify that you get a timeout on a Simulator with no Developer Account signed in on a more or less blank macOS instance without any virtualization. @nriedman can you verify this on your machine as well? As far as I remember you also weren't signed into your developer account on Xcode.

@PSchmiedmayer
Copy link
Member

I think the best approach here would be to document this behavior (Apple doesn't seem to do that for us ...) and then ensure that clients that use this can catch the Timeout error and catch it appropriately for UI testing behavior (e.g. for our apps using the testing flag to ignore this specific error). I am worried that giving an invalid token that we generate ourselves will lead to too many unexpected behavior down the line and might confuse developers even more ... what is your though on this @Supereg?

@nriedman
Copy link

Hi @Supereg and @PSchmiedmayer! I checked out the TestApp on this branch with no development account (I signed out on Xcode and cleared the simulator to make sure no developer account could sneak in there), and I did receive a device token when I selected "Register". It seems to work as intended on my machine. I'm not sure why this is the case, when it doesn't work on the CI or on @PSchmiedmayer's test, but I can confirm that it works for me both with a personal developer account and no developer account at all.

@Supereg
Copy link
Member Author

Supereg commented Aug 26, 2024

@PSchmiedmayer I went forward and documented the behavior (as far as we could reproduce it) in the docs. Additionally I we log a warning when throwing a timeout error due to timing out on a simulator device. I also covered catching the TimeoutError in simulator environment the code example so this is easily discoverable. What do you think?

@Supereg
Copy link
Member Author

Supereg commented Aug 26, 2024

Markdown lint is currently failing due to a SSL error when requesting https://reuse.software/spec

@PSchmiedmayer
Copy link
Member

Nice; thank you for fixing all the issues and adding the documentation! Happy to merge it despite the MD check failing, we can't really fix the reuse.software HTTPs issues 😁

@PSchmiedmayer PSchmiedmayer merged commit f1f6fb4 into main Aug 26, 2024
13 of 15 checks passed
@PSchmiedmayer PSchmiedmayer deleted the fix/documentation-remote-notifications branch August 26, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants