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

[feature/better-sync] Sync Engine fixes and improvements #1351

Closed
wants to merge 14 commits into from

Conversation

felix-schwarz
Copy link
Contributor

@felix-schwarz felix-schwarz commented Apr 29, 2024

Description

This PR includes the following fixes and improvements to the Sync Engine in the SDK:

It also fixed and improves the following in the app itself:

  • cancellation of actions using the inline progress indicator of items works again
  • fix bug where a folder selected for Available Offline could not be removed from Available Offline by action again

Further opportunities for exploration and improvement:

  • cross-process progress reporting (possibly via OCSignals)
  • allow re-scheduling sync actions
  • media upload reliability
  • activity reporting/display/interaction (switch to data sources + take advantage of collection view)

Related Issue

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@felix-schwarz felix-schwarz self-assigned this Apr 29, 2024
@CLAassistant
Copy link

CLAassistant commented Apr 29, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ hosy
❌ felix-schwarz
You have signed the CLA already but the status is still pending? Let us recheck it.

- ProgressView: add minimum + touch friendly sizes to avoid display as zero-sized views (which prevented cancelling by tapping because the touch never reached the view)
- AvailableOfflineAction: fix bug where a folder selected for Available Offline could not be removed form Available Offline by action (due to Swift != comparison apparently not using -isEqual:)
@jesmrec
Copy link
Contributor

jesmrec commented Aug 14, 2024

Checks to do

Uploads

  • Upload 50 files from Photo albums (oCIS)
  • Upload 200 files from Photo albums (oCIS)
  • Upload 300 files from Photo albums (oCIS)
  • Upload 350 files from Photo albums (oCIS)
  • ..

@jesmrec
Copy link
Contributor

jesmrec commented Aug 14, 2024

(1)

I'm having some problems to perform bulk uploads

  1. Select + and Upload from your photo library
  2. Select 50 pictures/videos -> it starts to import and upload
  3. At some point, the media import stops in 26/50 and some uploads seem to be stucked
  4. Killing the app and reopening, i got a SQLite error a couple of times, that was fixed on a new attempt some minutes later
  5. After opening the app, only 26 files are uploaded and "Status" is clear.

Expected: 50 files uploaded. For bigger amount of files, the problem gets worse.

I caught a log file over these steps:

ownCloud_14_Aug_2024_at_12_15_54.log.txt

and also, a short footage:

RPReplay_Final1723630629.MP4

NOTE: Status is clear, but, after trying another upload, the pending files appear again in the Media Import together with the new ones. I mean, if i select another 100 files to upload, i see:

Screenshot 2024-08-14 at 12 39 18

that are the new 100 uploads plus 24 that were lost before (only uploaded 26 out of 50). It is stucked in there (3 of 124) with no advance. After killing and reopening the app, Status is clear again and no new file was uploaded.

iPhone XR iOS 17.6.1
7837e1bca
oCIS 5.0.6

@felix-schwarz
Copy link
Contributor Author

felix-schwarz commented Aug 15, 2024

@jesmrec Thanks for the log and screen recording! Excellent basis for debugging 😃

After a first glance at the logs, it appears that

  • the FP appears to block the database for a prolonged time when you see the "busy" messages
  • the File Provider crashed around 2024-08-14 12:21:19.677000+0200 (it starts logging anew at this point)
  • the uploads already scheduled on the File Provider (which Media Upload does) don't continue for the moment because the app is expecting the File Provider to perform them
  • after the app is relaunched, the app takes a new look at the Sync Queue, realizes the original process (File Provider) is not around anymore, takes them over and finishes the scheduled uploads (2024-08-14 12:24:03.816000+0200 and later). Disconnecting and re-connecting in the app might have the same effect.

The improvements in this branch focused on improving uploads in general and didn't include Media Upload.

Background: Media Upload is not part of the SDK (where this branch focused on improvements), maintains its own queue of photo/video items to export and convert from the Photo Library - and only then schedules uploads.

The uploads that were scheduled went through as far as I can tell. I certainly need to spend some time to debug Media Upload next and see what's causing the issues you observed here, but that's outside the scope of this PR at least.

@jesmrec
Copy link
Contributor

jesmrec commented Aug 16, 2024

OK, as testing task for the uploads, i put on my desk a test plan that checks that all kind of uploads work as expected. Starting for media, but also uploads from Files app, auto uploads or uploads from 3rd party apps. I don't find any other way to check this... i will resume the work.

@felix-schwarz
Copy link
Contributor Author

I found that the File Provider appears to almost immediately be killed on iPadOS 18 because it runs out of the 20 MB memory at launch time (after 1 ms), which makes debugging it almost impossible.

Recalling that newer OpenSSL versions now load cipher suites and strings automatically, I checked whether it was linked into the File Provider and - indeed - it's included via the ownCloudApp.framework, of which it needs only a few parts.

So, in order to remove OpenSSL from the File Provider as well as all other unneeded parts, I changed the project to build and link the necessary parts from ownCloudApp.framework directly into the File Provider now, which appears to have given the File Provider enough breathing room for regular use via Files.app.

However, since Media Upload should be rock solid and the File Provider might still run out of memory depending in the size of an account, I disabled upload via File Provider for it.

All of the above changes are in #1376 now.

- fix protocol warnings new in Xcode 16
- adopt newer #unavailable syntax instead of if #available { /* Nothing */ } else { /* Code*/ }
@jesmrec
Copy link
Contributor

jesmrec commented Aug 20, 2024

New checks performed:

  • Folder with 3000 files inside and another folder with another 3000, set as av. offline
    • In a row. Some performance issues ⚠️
    • Terminating the app in the middle
  • Folder with hundred files that contains other folder with other hundred.
    • In a row. Some performance issues
    • Terminating the app in the middle

@jesmrec
Copy link
Contributor

jesmrec commented Aug 21, 2024

@felix-schwarz i performed some checks based in the cases listed just above 👆 , result was OK.

I just noticed that very populated folders (3000 files inside or more) sometimes does a slow navigation. An spinner while loading the list of files could be a good idea (out of scope here).

As agreed, regression testing in release phase will test also other cases.

@jesmrec jesmrec mentioned this pull request Sep 4, 2024
6 tasks
@felix-schwarz
Copy link
Contributor Author

Changes from this PR were also included in #1376 and merged that way.

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.

4 participants