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

Background NSURLSession behaviour #46

Closed
felix-schwarz opened this issue Apr 11, 2019 · 6 comments
Closed

Background NSURLSession behaviour #46

felix-schwarz opened this issue Apr 11, 2019 · 6 comments

Comments

@felix-schwarz
Copy link
Collaborator

Following reports by @hosy and @tomneedham, I've been investigating the behaviour of background NSURLSession, which all evidence so far pointed to as the cause for huge delays and a seemingly stuck Sync Engine.

Findings

Here's what I found after running into the issue during testing myself:

  1. The download request for San Francisco.jpg is scheduled in the NSURLSession at 2019-04-10 13:41:40 by way of calling the NSURLSessionTasks resume method.

IMG_8450

  1. The download returns at 2019-04-10 13:49:35 - almost 8 minutes later! The look at metrics shows that the NSURLSession did in fact wait so long before actually sending the request:

IMG_8448

Possible cause

Going through Apple's documentation on background NSURLSessions, this paragraph looks relevant:

IMG_8452

(Source)

Except that the app did not actually fulfill the criteria and stayed in the foreground all the time - and requests also didn't start immediately when putting the app in the background and foreground again (which, according to this doc, should reset any penalty).

My best guess is that the FileProvider's use of background NSURLSessions is somehow counted as background activity towards the app itself - and leads to penalties even if the app is in the foreground.

The documentation also points out the existence of a Background Networking profile.

I installed that profile, reproduced the issue and then took the sysdiagnose recommended by the accompanying instructions. However, I could not find anything relevant in it yet.

As far as a solution is concerned, I'll try following this advice from Apple's documentation:

You don’t have to do all background network activity with background sessions as described in this article. Apps that declare appropriate background modes can use default URL sessions and data tasks, just as if they were in the foreground.

That is, except for the File Provider, which Apple explicitly tells us should use background sessions.

I'll update this issue as work on it progresses.

@felix-schwarz
Copy link
Collaborator Author

felix-schwarz commented Apr 15, 2019

Another interesting observation: the background NSURLSession used by the File Provider does not seem to be affected in any way. Just the background NSURLSession that was used by the app.

@felix-schwarz
Copy link
Collaborator Author

Some interesting insights here: http://c-sharx.net/2015-11-25-ios-background-transfer-what-about-uploads

@felix-schwarz
Copy link
Collaborator Author

felix-schwarz commented Apr 16, 2019

Upon further inspection, I found two places where the app might - other than thought previously - actually have fulfilled the criteria for being penalized:

  • background item list updates: these were using requests scheduled with an earliestBeginDate date in the future and, when that future date arrived, scheduling new requests immediately after getting the response
  • uploads: uploads were scheduling a PROPFIND on the uploaded item right after finishing the upload

Solutions I'm putting in place:

  • background item list updates now use a local NSURLSession:
    • using OCBackgroundAction to keep the app awake in the background for already scheduled requests
    • using OCBackgroundManager to schedule sending these requests to the local queue only when the app is in the foreground (a new feature of OCBackgroundManager can queue blocks if the app is backgrounded - and runs them once the app is in foreground again)
  • single-item PROPFINDs after a successful upload now also use a local NSURLSession, avoiding the danger of triggering penalties from iOS

felix-schwarz added a commit to owncloud/ios-app that referenced this issue Apr 16, 2019
- Fix issue (30) in #237 with fixes described in owncloud/ios-sdk#46 (comment)
@felix-schwarz
Copy link
Collaborator Author

More findings:

  • Setting the NSURLSessionConfiguration.networkServiceType to NSURLNetworkServiceTypeResponsiveData seems to increase responsiveness
  • NSURLSession seems to behave differently when run on the device via Xcode vs. when launched by the user on the device

@felix-schwarz
Copy link
Collaborator Author

One of the insights gained from the blog post at http://c-sharx.net/2015-11-25-ios-background-transfer-what-about-uploads is that background NSURLSessions seem to have a global transfer budget, so that it's desirable only to use the background NSURLSession if using a local NSURLSession is not an option.

As of cead9cf, OCHTTPPipeline saves metrics and can provide estimates (complete with confidence levels) based on that. That estimate is then used by OCConnection to make a choice between local and background NSURLSession: if a transfer is likely to take less than two minutes (safely within the boundaries of available background execution time) with at least 15% confidence, it'll use the local session. More details in the commit message of cead9cf.

michaelstingl pushed a commit to owncloud/ios-app that referenced this issue Apr 29, 2019
* hide beta warning

* [Bugfix 1.0.0] Fixed an issue with drag to action not working for some actions

* - Fix issue (3) "Oversized thumbnails"
- Fix separator insets in item list

* - Produce an appropriate error when an upload fails due to insufficient space left on the server (fix (4))

* - Fixing issue (1) and (2) via SDK update

* Version Bump to 114

* - disable table cell selection
- fixed long status messages, while setting dynamically numbers of lines

* Version Bump to 115

* - Fixed (5) where the progress of a file that completed download in the background was shown incorrectly

* - Improve progress updates and representation in ClientItemCell

* - Fix: indicate activity during last phase of setting up a new bookmark and whenever editing a bookmark and saving it (previously UI seemed stuck for a couple of seconds)

* - Added NSError+MessageResolution that resolves the localized description for OCErrors so they are included when serialized, so they become visible in the Files app, too
- Fixing issue (8) in #237

* Implemented improvement (13)

* - Fix newly introduced potential crash bug in renameItem FP method
- Remove trash method and permission flag for items in FP, so "Delete Now" is the only option and Files app shows a warning
- Fixing (11) in #237

* - Rename uploadsBarButton to plusBarButton in ClientQueryViewController
- Add new action extension location .plusButton
- Add Action extensions support to provide UIAlertAction objects
- Add .plusButton location to CreateFolder action
- Extend the ClientQueryViewController plusBarButton to include action extensions in the alert that's being shown
- Implementing improvement (15) from #237

* Fixed (13): restore multiple selection after reloading file list

* - Move File Upload and Photo Upload from ClientQueryViewController to distinct Action Extensions
- Extend ActionProgressHandler to cater for both publishing and unpublishing progress objects
- Replace repeated code with a method to provide a ActionProgressHandler in ClientItemQueryViewController
- Make CreateFolderAction require a folder as parent item
- Make MoveAction present on the provided ViewController, not its navigationController
- Extend error handling in several places

* Fixed (21): folder picker not canceleable when navigating into folder and back

* - SortBar overhaul / cleanup
	- remove create folder button from left
	- left-align the sort option and rename it "Sort by" (from "Sorted by")
	- turn the sort option button from a ThemeButton to a UIButton and replace the unicode arrow with an elegant chevron
	- remove sortBar location for action extensions
	- clean up SortBar code
- NSObject+ThemeApplication
	- replace use of isKindOfClass and force-casts with if let constructs
	- add support for UIButton themeing
- fix "flashing" reording right after opening a connection by setting the OCQuery sort method before starting the query
- fix bug where an alert was shown for invalid credentials, but the Edit button did not work
	- fix ClientRootViewController.closeClient(), so it calls the completionHandler
	- adjust the editing method to the changed view and navigation controller structure
- minor code formatting and style fixes in several places

* Fixed (22): no ‘more’ button in directory picker and now swipe to delete

* Another small fix for (22)

* Fix for multi-selection regression (items automatically selected) when re-entering multi-selection

* - Fix UI main thread violation in BookmarkViewController
- Fix spelling of CreateFolderAction.identifier value
- Simplify implementation of DuplicateAction by refactoring it to use new SDK APIs, addressing issue (7) in #237
- Fix potential future crash in ClientDirectoryPickerViewController by avoiding force-casting
- Make OCItem.parentItem in OCItem+Extension more robust using new SDK API
- Adapt FileProviderExtension to use new, supported SDK APIs instead of the island implementation that was -findKnownExistingItemInParent:withName:
- Add (and comment out) code that lets FileProviderExtension.createDirectoryWithName return as soon as the placeholder item is available
- Add empty implementations for trashItemWithIdentifier/untrashItemWithIdentifier/setLastUsedDate/supportedServiceSourcesForItemIdentifier to comply with requirement of overriding all methods raised in the documentation

* Fix for (24) refresh animation glitch

* Fixed (26) crash when trying to downsample corrupt image file

* Version Bump to 116

* Fixed (27): not downloading files which are present offline

* Using OCCore.localCopy() to test if the file is already downloaded

* Another fix for (27)

Not dismissing DisplayViewController when OpenIn action is triggered.

* - Update SDK

* - Address issue (17) from #237 by bringing up a custom error message when a user likely attempted a cross-server move operation through the Files app

* fixed earl grey ui test failures, because UI or logic changed in release_1.0.0 fixes

* - Address issue (30) in #237 via SDK update

* fixed memory leak, which causes that the viewcontroller was not released (this was recognized, because audio or video keeps playing in the background)

* - Updating SDK, fixing issues described for the initial release issue (30) fix

* - Fix issue (19) in #237
- Fix issue (30) in #237 with fixes described in owncloud/ios-sdk#46 (comment)

* - Addressing (30) in #237

* Fixed (35) handling taps in the PDFThumbnailsView

* Some fixes regarding thumbnail size calculation

* - Update to latest SDK (to solve (30))

* Fixed (31) plus couple of PDF thumbnail view fixes

* Fixed (37) disable horizontal scrolling in the gallery if there is a single item

* - Update ios-sdk which now includes important fixes from the ios-sdk feature/sharing branch

* - Fix another formatting warning

* - Certificate Manager now shows the acceptance reason for user-accepted certificates
- Certificate View now shows the acceptance reason description for user-accepted certificates

* Few cosmetic changes

* Removed unused instance variable

* Fixed (38) tap to hide bars -> recalculate image size

* Fixed an issue with swipe in the image gallery causing navigation bar to reappear

* Fixed (39) Going back to root folder when selected “Browse” tab is tapped

* - Fixing two issues from #237:
	- (25) Double deletion in maintenance mode
        - (41) Files re-appear in the files list after batch-deleting them

* - Fix (42) in #237: considerable CPU use and slow-down in busy/frequently updated directories

* - Fixing (33) in #237 via SDK update: give users option to keep file or retry when upload fails

* - Fix issue (32) and (40) in #237 by presenting ClientRootViewController
	- only when the core has been returned by OCCoreManager, showing an activity indicator in the selected cell until then
	- modally, using a custom push transition to avoid UINavigationController hell
- Fix issue where ClientRootViewController would dismiss the lat ClientQueryViewController while also being dismissed, leading to an inconsistent animation when looking closely

* - Cleanup

* - Cleanup

* Version Bump to 117

* Fixed UI tests which broke due to changes made to fix (32) and (40)

* Fixed remaining UI tests

* - Fix issue where logging in and out of a bookmark in quick succession could lead to hangs opening the bookmark (likely is (32) in #237)

* - Fix failing UI tests

* - Make certificate detail view more resilient

* - Update SDK to add logging around OCCoreManager core requests/returns

* - SDK update:
        - Make OCCoreManager use an administrative queue for every bookmark, so that one connection closing down or opening up can't hold up another
- Takes advantage of OCCoreManager managing every bookmark independently
	- allowing to select another bookmark if the previous one takes longer to open
	- not show the UI for a bookmark if Settings, Help or Add/Edit Bookmark UI is triggered meanwhile
	- fix crash caused by force-unwrapping self.core in ClientRootViewController.coreReady()

* - Address "(44) Re-save file to the same folder will lead to an error" in #237

* - Avoid showing download progress indicator when viewing files that are already downloaded (issue (46) in #237)
- Fix bug in PDFViewerViewController where CoreGraphics would throw an error when using pdfView with zero size (before properly laying it out)

* - Clean up DisplayViewController and remove force-casts
- Fix (47) "the non-openable files prompt the download progress bar in the "details" view" in #237

* removed localization for "Feedback" button

* Feature/heic to jpeg (#363)

* Added HEIC to JPEG conversion

* Localized strings for photo upload settings

* Fixed indentation

* Fixed license header

* Version Bump to 118

* Made sure that photo metadata is exported
michaelstingl pushed a commit to owncloud/ios-app that referenced this issue May 13, 2019
* hide beta warning

* [Bugfix 1.0.0] Fixed an issue with drag to action not working for some actions

* - Fix issue (3) "Oversized thumbnails"
- Fix separator insets in item list

* - Produce an appropriate error when an upload fails due to insufficient space left on the server (fix (4))

* - Fixing issue (1) and (2) via SDK update

* Version Bump to 114

* - disable table cell selection
- fixed long status messages, while setting dynamically numbers of lines

* Version Bump to 115

* - Fixed (5) where the progress of a file that completed download in the background was shown incorrectly

* - Improve progress updates and representation in ClientItemCell

* - Fix: indicate activity during last phase of setting up a new bookmark and whenever editing a bookmark and saving it (previously UI seemed stuck for a couple of seconds)

* - Added NSError+MessageResolution that resolves the localized description for OCErrors so they are included when serialized, so they become visible in the Files app, too
- Fixing issue (8) in #237

* Implemented improvement (13)

* - Fix newly introduced potential crash bug in renameItem FP method
- Remove trash method and permission flag for items in FP, so "Delete Now" is the only option and Files app shows a warning
- Fixing (11) in #237

* - Rename uploadsBarButton to plusBarButton in ClientQueryViewController
- Add new action extension location .plusButton
- Add Action extensions support to provide UIAlertAction objects
- Add .plusButton location to CreateFolder action
- Extend the ClientQueryViewController plusBarButton to include action extensions in the alert that's being shown
- Implementing improvement (15) from #237

* Fixed (13): restore multiple selection after reloading file list

* - Move File Upload and Photo Upload from ClientQueryViewController to distinct Action Extensions
- Extend ActionProgressHandler to cater for both publishing and unpublishing progress objects
- Replace repeated code with a method to provide a ActionProgressHandler in ClientItemQueryViewController
- Make CreateFolderAction require a folder as parent item
- Make MoveAction present on the provided ViewController, not its navigationController
- Extend error handling in several places

* Fixed (21): folder picker not canceleable when navigating into folder and back

* - SortBar overhaul / cleanup
	- remove create folder button from left
	- left-align the sort option and rename it "Sort by" (from "Sorted by")
	- turn the sort option button from a ThemeButton to a UIButton and replace the unicode arrow with an elegant chevron
	- remove sortBar location for action extensions
	- clean up SortBar code
- NSObject+ThemeApplication
	- replace use of isKindOfClass and force-casts with if let constructs
	- add support for UIButton themeing
- fix "flashing" reording right after opening a connection by setting the OCQuery sort method before starting the query
- fix bug where an alert was shown for invalid credentials, but the Edit button did not work
	- fix ClientRootViewController.closeClient(), so it calls the completionHandler
	- adjust the editing method to the changed view and navigation controller structure
- minor code formatting and style fixes in several places

* Fixed (22): no ‘more’ button in directory picker and now swipe to delete

* Another small fix for (22)

* Fix for multi-selection regression (items automatically selected) when re-entering multi-selection

* - Fix UI main thread violation in BookmarkViewController
- Fix spelling of CreateFolderAction.identifier value
- Simplify implementation of DuplicateAction by refactoring it to use new SDK APIs, addressing issue (7) in #237
- Fix potential future crash in ClientDirectoryPickerViewController by avoiding force-casting
- Make OCItem.parentItem in OCItem+Extension more robust using new SDK API
- Adapt FileProviderExtension to use new, supported SDK APIs instead of the island implementation that was -findKnownExistingItemInParent:withName:
- Add (and comment out) code that lets FileProviderExtension.createDirectoryWithName return as soon as the placeholder item is available
- Add empty implementations for trashItemWithIdentifier/untrashItemWithIdentifier/setLastUsedDate/supportedServiceSourcesForItemIdentifier to comply with requirement of overriding all methods raised in the documentation

* Fix for (24) refresh animation glitch

* Fixed (26) crash when trying to downsample corrupt image file

* Version Bump to 116

* Fixed (27): not downloading files which are present offline

* Using OCCore.localCopy() to test if the file is already downloaded

* Another fix for (27)

Not dismissing DisplayViewController when OpenIn action is triggered.

* - Update SDK

* - Address issue (17) from #237 by bringing up a custom error message when a user likely attempted a cross-server move operation through the Files app

* fixed earl grey ui test failures, because UI or logic changed in release_1.0.0 fixes

* - Address issue (30) in #237 via SDK update

* fixed memory leak, which causes that the viewcontroller was not released (this was recognized, because audio or video keeps playing in the background)

* - Updating SDK, fixing issues described for the initial release issue (30) fix

* - Fix issue (19) in #237
- Fix issue (30) in #237 with fixes described in owncloud/ios-sdk#46 (comment)

* - Addressing (30) in #237

* Fixed (35) handling taps in the PDFThumbnailsView

* Some fixes regarding thumbnail size calculation

* - Update to latest SDK (to solve (30))

* Fixed (31) plus couple of PDF thumbnail view fixes

* Fixed (37) disable horizontal scrolling in the gallery if there is a single item

* - Update ios-sdk which now includes important fixes from the ios-sdk feature/sharing branch

* - Fix another formatting warning

* - Certificate Manager now shows the acceptance reason for user-accepted certificates
- Certificate View now shows the acceptance reason description for user-accepted certificates

* Few cosmetic changes

* Removed unused instance variable

* Fixed (38) tap to hide bars -> recalculate image size

* Fixed an issue with swipe in the image gallery causing navigation bar to reappear

* Fixed (39) Going back to root folder when selected “Browse” tab is tapped

* - Fixing two issues from #237:
	- (25) Double deletion in maintenance mode
        - (41) Files re-appear in the files list after batch-deleting them

* - Fix (42) in #237: considerable CPU use and slow-down in busy/frequently updated directories

* - Fixing (33) in #237 via SDK update: give users option to keep file or retry when upload fails

* - Fix issue (32) and (40) in #237 by presenting ClientRootViewController
	- only when the core has been returned by OCCoreManager, showing an activity indicator in the selected cell until then
	- modally, using a custom push transition to avoid UINavigationController hell
- Fix issue where ClientRootViewController would dismiss the lat ClientQueryViewController while also being dismissed, leading to an inconsistent animation when looking closely

* - Cleanup

* - Cleanup

* Version Bump to 117

* Fixed UI tests which broke due to changes made to fix (32) and (40)

* Fixed remaining UI tests

* - Fix issue where logging in and out of a bookmark in quick succession could lead to hangs opening the bookmark (likely is (32) in #237)

* - Fix failing UI tests

* - Make certificate detail view more resilient

* - Update SDK to add logging around OCCoreManager core requests/returns

* - SDK update:
        - Make OCCoreManager use an administrative queue for every bookmark, so that one connection closing down or opening up can't hold up another
- Takes advantage of OCCoreManager managing every bookmark independently
	- allowing to select another bookmark if the previous one takes longer to open
	- not show the UI for a bookmark if Settings, Help or Add/Edit Bookmark UI is triggered meanwhile
	- fix crash caused by force-unwrapping self.core in ClientRootViewController.coreReady()

* - Address "(44) Re-save file to the same folder will lead to an error" in #237

* - Avoid showing download progress indicator when viewing files that are already downloaded (issue (46) in #237)
- Fix bug in PDFViewerViewController where CoreGraphics would throw an error when using pdfView with zero size (before properly laying it out)

* - Clean up DisplayViewController and remove force-casts
- Fix (47) "the non-openable files prompt the download progress bar in the "details" view" in #237

* removed localization for "Feedback" button

* Feature/heic to jpeg (#363)

* Added HEIC to JPEG conversion

* Localized strings for photo upload settings

* Fixed indentation

* Fixed license header

* Version Bump to 118

* Made sure that photo metadata is exported

* - ownCloud iOS SDK and Xcode update

* Fixes for #365

- Offloaded photo import operations off the main thread
- Using fullsize image URL to derive filename

* Memory use improvements

* - Update SDK
- include xcscheme options for new configuration variables

* - Correct settings in scheme

* More memory handling improvements when dealing with image conversion

* - ProgressSummarizer updates
	- now groups progress by type and provides summaries such as "Uploading X of Y files.." that are easier to follow for the user
	- now prioritizes newer progress submissions over old ones, taking into account the possbility these may be executed earlier due to Sync Engine concurrency

* - Clean up Viewer code
	- Remove unused original DisplayHostViewController
	- Rename GalleryHostViewController to DisplayViewController
	- move Viewer code below Client where it logically belongs and split it up into subdirectories
- (Re-)add download progress reporting to ClientQueryViewController and initially add it to Display*ViewController classes

* - ios-sdk update adding Sync Action categories and concurrency budgets
- Clean ups

* - Switch back to NSLog from Log.swift in tests (Log.swift is not incuded in the test target) so tests pass
- Remove XCTestsCase+Extension as it provides no benefit over Xcode's own messages whn tests start and finish
@felix-schwarz
Copy link
Collaborator Author

Closing this issue as it has been addressed at the time of writing.

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

No branches or pull requests

1 participant