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 bug where HTTP requests were incorrectly cancelled #168

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

FelixSFD
Copy link

hopefully fixes #167


First, I changed the HTTP call from .data() to .dataTask() with a completion handler. I did this, because I couldn't find out, which exact line was throwing the exception, when the request was cancelled.
Because I didn't want to change the rest of the class, I wrapped this new call inside withCheckedThrowingContinuation() to turn this into an async call.
This didn't fix the issue completely, because there was a new warning in the log:

SWIFT TASK CONTINUATION MISUSE [...] leaked its continuation!

This error told me, what happened to the running tasks when navigating quickly: Something is wrong with the memory management of this class.
So I switched the property grocyVM in the views from @SteteObject to @ObservedObject, which is recommended for properties that are not created inside the view, but injected instead: https://www.avanderlee.com/swiftui/stateobject-observedobject-differences/#should-i-use-stateobject-for-all-views-using-the-same-instance

@FelixSFD FelixSFD marked this pull request as ready for review May 28, 2023 11:05
@FelixSFD
Copy link
Author

@supergeorg tested it in the simulator and on my iPhone 13 mini and the bug seems to be fixed without breaking anything. But I think it's a good idea, if an experienced user would test as well, because I'm quite new to Grocy and might have missed something.

@supergeorg
Copy link
Owner

Well, I am unsure if this is the best solution. This works because the cancellation is not implemented.
This seems to work, but I have found a different way (wrapping task into task, 5b2c45b) which seems to work too.

I will check if ObservedObject is better than StateObject and will adapt the code accordingly.
If my solution doesn't work, I will be happy to merge yours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unknown error when navigating
2 participants