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

Resumable download #400

Merged
merged 32 commits into from
Aug 20, 2024
Merged

Resumable download #400

merged 32 commits into from
Aug 20, 2024

Conversation

yanisoln
Copy link
Contributor

@yanisoln yanisoln commented Jul 5, 2024

For Android and iOS :

This pull request introduces a resumable download feature to the updater, addressing the issue reported in #255.

The new functionality ensures that if the application is closed, receives a SIGKILL signal, or crashes during an update, the update process will resume from the last completed point the next time the application is launched.

This enhancement aims to provide a more robust update experience by mitigating the impact of disruptions during the update process while reducing bandwidth usage.

You can see it in action here : https://www.youtube.com/watch?v=Y-MFS3PfmCc

Let me know if any question !

/claim #255

…n case of crash or app closing during update
Copy link

algora-pbc bot commented Jul 5, 2024

💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe.

@riderx
Copy link
Collaborator

riderx commented Jul 5, 2024

Thanks for the work, this should be done in Android and IOS, can you work on the ios side?

@yanisoln
Copy link
Contributor Author

yanisoln commented Jul 5, 2024

Thanks for the work, this should be done in Android and IOS, can you work on the ios side?

Hi, I'm currently working on it

@yanisoln yanisoln force-pushed the resumable_download branch from 0ac684a to 37efd56 Compare July 6, 2024 11:34
@yanisoln
Copy link
Contributor Author

yanisoln commented Jul 6, 2024

Just added resumable download for iOS in 37efd56

You can see the video : https://www.youtube.com/watch?v=_QO9jNQHbIQ

@riderx
Copy link
Collaborator

riderx commented Jul 6, 2024

Thanks for the work!
@neo773 and @ayewo if you are around to help on the review, I can support both :)
As it's critical for Capgo

@ayewo
Copy link
Contributor

ayewo commented Jul 7, 2024

... if you are around to help on the review, I can support both :) As it's critical for Capgo

Hey @riderx 👋!

I don't see a sibling PR from the author for the Cap-go/capgo/ repo so I need to ask clarifying questions since I've been away from the code base for a while now.

Regarding my earlier comment:

  1. It seems a separate endpoint is no longer needed now that the expiration time for a download URL from S3 is now 1 week instead of 2mins?

  2. Is the service still using Cloudflare caching?

@riderx
Copy link
Collaborator

riderx commented Jul 7, 2024

@ayewo you are correct we now extended the validity of the zip as it was required for other cache test.
We do use cloudflare r2, but not the cache i think
In the future i think we will even remove the singing to be able to benefit from CND of cloudflare r2 with custom domain name.

@ayewo
Copy link
Contributor

ayewo commented Jul 10, 2024

Hi @YanisOUALAN,

can you please provide step-by-step instructions that can be used to verify your implementation works as desired?

Including the demo app you used for testing would be nice too.

Thanks!

@yanisoln
Copy link
Contributor Author

yanisoln commented Jul 11, 2024

Hi @YanisOUALAN,

can you please provide step-by-step instructions that can be used to verify your implementation works as desired?

Including the demo app you used for testing would be nice too.

Thanks!

Hey, sure,

To setup the demo app:

  1. Download the app here
  2. Extract it and all the stuff
  3. Install capacitor with npm init @capacitor/app

Install my version of capgo :

  1. Run in the demo app folder the command git clone https://github.com/YanisOUALAN/capacitor-updater.git
  2. Move in the repo with cd capacitor-updater
  3. Move to the resumable_download branch with git checkout resumable_download
  4. Run npm install and npm run build
  5. Create a symbolic link with npm link
  6. Go back in the demo app folder with cd ../ and add reference to the symbolic link with npm link @capgo/capacitor-updater then run npm run build

Create a bundle for triggering the update :

  1. Give to the app a unique package name in capacitor.config.ts
  2. Init the capgo setup with the command on your capgo cloud dashboard or setup your self hosted solution. When it ask to Automatic Install "@capgo/capacitor-updater" dependency say no, otherwise it will erase my version of capgo. Same thing goes when it ask if you want to add Automatic Add "CapacitorUpdater.notifyAppReady()" code and import say no as it's already added
  3. Add your platform with npx cap add android and/or npx cap add ios
  4. Run npm run build and npx cap sync
  5. If you're using capgo cloud, setup the production channel as the default channel
  6. Modify in demo app package.json the attribute "version": "1.1.x", with "version": "1.1.x+1", you can also modify the texts in src/App.js file to make the change more clear when the app is updated
  7. Run the command npm run build
  8. Upload your bundle to trigger an update the next time the app is launched, if you're using capgo cloud, use the command npx @capgo/cli@latest bundle upload --channel=production
  9. If you're using capgo cloud, setup the production channel as the default channel
  10. Test your app on Android Studio or xCode with npx cap open android or/and npx cap open ios

For testing (iOS and Android) :

The termination (SIGKILL) scenario :

  1. Build & launch the demo app
  2. Wait for the download to start downloading
  3. Before the download has reached 100%, kill the app (as i did in both videos i showed before)
  4. Restart the app
  5. Ensure that the download is not restarting from 0 but from where it was before the app get killed
  6. Once the download has ended, restart again the app and ensure that it has been updated

The disconnected scenario :

  1. Build & launch the demo app
  2. Wait for the download to start downloading
  3. Before the download has reached 100%, disconnect your emulator/your host from internet
  4. Close the app
  5. Bring your connection back
  6. Restart the app
  7. Ensure that the download is not restarting from 0 but from where it was before the app get disconnected
  8. Once the download has ended, restart again the app and ensure that it has been updated

The app closed scenario :

  1. Build & launch the demo app
  2. Wait for the download to start downloading
  3. Before the download has reached 100%, open the multitasking menu on your device (swipe the navigation bar up) and close the app (by sliding it up)
  4. Restart the app
  5. Ensure that the download is not restarting from 0 but from where it was before the app get closed
  6. Once the download has ended, restart again the app and ensure that it has been updated

I suggest to add these lines to facilitate the debugging in node_modules/@capgo/capacitor-updater/android/src/main/java/ee/forgr/capacitor_updater/DownloadServices.java capgo file just after int percent = calcTotalPercent(downloadedBytes, contentLength); on line 118 :

System.out.print("Download progress : " + percent);

And on node_modules/@capgo/capacitor-updater/ios/Plugin/CapacitorUpdater.swift, just after let percent = Int((Double(totalReceivedBytes) / Double(targetSize ?? 1)) * 100.0) on line 628 :

print("Downloading : \(percent)%")

In order to show the download progress

I also recommend using a network throttler like NetLimiter for Windows or Network Link Conditioner for MacOS so you can slow down the download progress artificially, otherwise it will happen too quickly before you can try to stop it.

If the download happened and your app got updated and you want to test the downloading process again, just go to package.json in the demo app folder, and change "version": "1.1.x", to "version": "1.1.x+1", run npm run build and upload a new bundle with npx @capgo/cli@latest bundle upload --channel=production note that it will just trigger the download process again, the react view and the associated text won't change, unless you change them.

Let me know if you have any question !

@riderx
Copy link
Collaborator

riderx commented Jul 18, 2024

Hey @ayewo do you have time to test it ?

@ayewo
Copy link
Contributor

ayewo commented Jul 18, 2024

Hey @riderx

Sorry for the delay. If it's not too late, I should have some availability to review it either tomorrow or Saturday, God willing.

@riderx
Copy link
Collaborator

riderx commented Jul 18, 2024

Yes it's good for us ! i will make it good pay as well as it's important

@yanisoln
Copy link
Contributor Author

Have you guys managed to test my implementation ? Let me know if you have any issues 😉 !

@riderx
Copy link
Collaborator

riderx commented Jul 23, 2024

@YanisOUALAN almost, @ayewo is sick it should be back soon

let semaphore: DispatchSemaphore = DispatchSemaphore(value: 0)
let id: String = self.randomString(length: 10)
var checksum: String = ""
//Do a GET request on the url in order to extract the Content-Length header, ask only for the 10 first bytes in order to not download the full content.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that you could use a "head" request in order to get the content-length. As far as I know we do that here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently, the server doesn't support HEAD requests; at least, when I tried to do so, I got a 403 response from the server, so I had to work around with GET and the Range header.
capgo head request

Copy link
Collaborator

@riderx riderx Aug 4, 2024

Choose a reason for hiding this comment

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

We do head request just here: https://github.com/Cap-go/capgo/blob/ebde1312a51feb6288ea5be750df71f3399cab46/supabase/functions/_backend/utils/s3.ts#L116
To get the size.
Just after getSignedUrl who is the function used to respond to the update (what you are getting), so it should work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please let me know if I'm missing anything. I attempted to use a HEAD request to retrieve the Content-Length header, but I kept receiving a 403 error, which suggests that the download URL may not support HEAD requests. To address this issue, in commit 90a09dd, I modified my GET request logic to include Range: bytes=0-0. This adjustment ensures that my GET request is basically the same as a HEAD request, effectively retrieving only the first byte and providing the necessary headers.
403capgo

@yanisoln yanisoln requested a review from riderx August 17, 2024 03:36
@WcaleNieWolny
Copy link
Contributor

Our download % is not only counting download but all steps, unzip and etc so it's important that the download even of pure download stay between 10 and 70 and the other are still present

@riderx I don't think that it's the case. I have not observed this behavior for a long while now. Currently download_xy does not work at all from what I've seen

I strongly believe that download_xy should be for download ONLY and not unzip, etc.
I believe we should make unzip a 0/1 function (either works or doesn't and we send an error)

@WcaleNieWolny
Copy link
Contributor

WcaleNieWolny commented Aug 17, 2024

@riderx we MUST start unifying the code base. This is a great opportunity to fix the download_%.
I was also thinking about backporting this change because it's a part of download and this PR fully rewrote download in IOS but we can do it in a later PR if you would like

@WcaleNieWolny
Copy link
Contributor

The thing about the download % is that we have NO WAY of knowing that it's going to be a multiple of 10.
In IOS, we don't control the chunk size so for small bundles + fast internet connections you may get the following:

percent: 12
percent: 53
percent 89
percent 100

I think that they are perfectly valid and that we should set them as a stat. I would say that we want to send a stat when the latest sent stat is a least 10 points different than the last stat that we sent like i did here

Copy link

@WcaleNieWolny
Copy link
Contributor

Alright, I would say that IOS is quite nicely polished. The one thing that is left in IOS is figuring out the download_% stat

@yanisoln
Copy link
Contributor Author

The thing about the download % is that we have NO WAY of knowing that it's going to be a multiple of 10. In IOS, we don't control the chunk size so for small bundles + fast internet connections you may get the following:

percent: 12 percent: 53 percent 89 percent 100

I think that they are perfectly valid and that we should set them as a stat. I would say that we want to send a stat when the latest sent stat is a least 10 points different than the last stat that we sent like i did here

As a possible solution, we could ensure every 10% milestone is recorded once a chunk completes. For instance, for a reported percent: 53, we would send :

  • download_0
  • download_10
  • download_20
  • download_30
  • download_40
  • download_50

if tracking precise milestones is crucial.

@WcaleNieWolny
Copy link
Contributor

We could, ultimately this decision is up to Martin.

Personally, i believe in sending it the way I proposed mostly because it conveys more information. If you see download in regular intervals (10, 20, 30) it means that the download is likely slow. If you see skips in the download (20 -> 50) that means that your bundle is really small and that it gets downloaded quickly

@riderx
Copy link
Collaborator

riderx commented Aug 17, 2024

@WcaleNieWolny i’m not sure to understand what you mean by it doesn’t work since long i see the events everyday in many users, i helped a customer yesterday with that.
I want this PR to keep the exact same behavior.
one task one PR :)

@WcaleNieWolny
Copy link
Contributor

@riderx here is the issue explained in details

2024-08-17.16-03-18.mp4

@riderx
Copy link
Collaborator

riderx commented Aug 19, 2024

Current system do send only for ten X percent so we want the same.
stats have to be same between os and version changing that is not planned.
so implementation of @YanisOUALAN for me is correct

@riderx riderx merged commit e1280f4 into Cap-go:main Aug 20, 2024
8 checks passed
@riderx
Copy link
Collaborator

riderx commented Aug 20, 2024

Hey @YanisOUALAN do you confirm your proposal was implemented?

@riderx
Copy link
Collaborator

riderx commented Aug 20, 2024

If not can you please do a quick PR to fix it, i didn’t saw it was missing

@yanisoln
Copy link
Contributor Author

Hey @YanisOUALAN do you confirm your proposal was implemented?

Hello, it wasn't implemented when this PR was merged since I didn't had confirmation if the behavior i suggested was the one needed, i am going to make another commit to implement my proposal later this day or tomorrow 👍

@riderx
Copy link
Collaborator

riderx commented Aug 20, 2024

Thanks 🙏
I think we got this issue today maybe related
Cap-go/capgo#804

@riderx
Copy link
Collaborator

riderx commented Aug 20, 2024

The issue reported is in android, did you had it working on android ?

@riderx
Copy link
Collaborator

riderx commented Aug 20, 2024

I merged your fix in ios

@yanisoln
Copy link
Contributor Author

The issue reported is in android, did you had it working on android ?

It did work, but the user reported that he used the 5.9.5 version, so I doubt that it's related to this PR

@riderx
Copy link
Collaborator

riderx commented Aug 21, 2024

ok great thanks i will check with the user

@riderx
Copy link
Collaborator

riderx commented Aug 22, 2024

@YanisOUALAN I think there is something odd: Cap-go/capgo#804
Why do signature is enforced? This is supposed to be only enforced if present in the config

@yanisoln
Copy link
Contributor Author

@YanisOUALAN I think there is something odd: Cap-go/capgo#804 Why do signature is enforced? This is supposed to be only enforced if present in the config

I don't remember changing anything about the signature, i am going to invest about this problem if you want

@riderx
Copy link
Collaborator

riderx commented Aug 22, 2024

@YanisOUALAN i think i found out and fixed it, it came from merge i think

@yanisoln
Copy link
Contributor Author

@YanisOUALAN i think i found out and fixed it, it came from merge i think

Oh okay, i applied a fix in #437, but if it has been resolved, then we can just close it i guess

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

Successfully merging this pull request may close these issues.

4 participants