-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix split install issue #2898
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 split install issue #2898
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these improvements. I think resumable downloads, and only downloading one split install package at a time, really make sense. However, I would prefer a different implementation approach: the way you are doing it is that the packages are first downloaded to a temporary directory, and then copied to the install session from this directory. (A previous iteration of the code also had this behavior, but we changed it to avoid occupying 2× the space of the installed package while installing it.)
I think it should be possible to instead resume downloads into the pending session. To do this:
- We need a registry of session IDs, so we know which download corresponds to which session.
- Sessions must not be discarded when download fails.
- We don't need temporary files anymore.
Do you see any reason that speaks against implementing it like this?
vending-app/src/main/kotlin/com/android/vending/installer/Install.kt
Outdated
Show resolved
Hide resolved
vending-app/src/main/java/org/microg/vending/ui/InstallProgressNotification.kt
Outdated
Show resolved
Hide resolved
vending-app/src/main/java/org/microg/vending/ui/InstallProgressNotification.kt
Outdated
Show resolved
Hide resolved
vending-app/src/main/java/org/microg/vending/delivery/Delivery.kt
Outdated
Show resolved
Hide resolved
vending-app/src/main/java/org/microg/vending/delivery/Delivery.kt
Outdated
Show resolved
Hide resolved
...ing-app/src/main/kotlin/com/google/android/finsky/splitinstallservice/SplitInstallManager.kt
Show resolved
Hide resolved
vending-app/src/main/kotlin/com/android/vending/installer/Install.kt
Outdated
Show resolved
Hide resolved
vending-app/src/main/kotlin/com/android/vending/installer/Install.kt
Outdated
Show resolved
Hide resolved
We temporarily store the downloaded files locally to avoid repeated downloads in case of issues, which could lead to unnecessary data usage for users. |
I don't know if there is such a limit. What do you expect the limit to be? What happens if it is exceeded? Do you expect to open many sessions, i.e. many partial and failed downloads? This would also mean a large storage consumption in either implementation, so it's not really desirable in either implementation. |
This is just an extreme case because Marvin initially raised concerns about data usage with me. That’s why I chose this approach. |
The session will even be persisted across multiple boots, please refer to: https://developer.android.com/reference/android/content/pm/PackageInstaller#createSession(android.content.pm.PackageInstaller.SessionParams) |
So do we need to store the sessions locally to facilitate reuse later? |
I think your solution still cannot meet the requirements for resumable downloads in special cases. |
Yes, as I mentioned, we could use a registry of session IDs, so we know which download corresponds to which session. Since we are session owner, we might also be able to use: https://developer.android.com/reference/kotlin/android/content/pm/PackageInstaller?hl=en#getStagedSessions() and compare the package name of the session. I suppose we might need to store how many bytes we wrote to the session, if it's not possible to ask the session that, though it might be feasible to use: https://developer.android.com/reference/kotlin/android/content/pm/PackageInstaller.Session?hl=en#openRead(kotlin.String) What special cases do you have in mind? |
I tried using your method, but I kept getting the following error when calling |
@DaVinci9196 Can you provide me with the code where this issue occurs? That will make it much easier for me to find out why that could be the case. |
Install.txt
|
@fynngodau Can you take a look? This is quite urgent. |
@DaVinci9196 I could not reproduce your problem, but I also do not have access to the
This worked without problems:
Does the above code already produce a problem for you? I noticed that you are never closing the session. I would suggest adding this to the |
This code cannot run on my phone and will report an error @fynngodau |
I have found that &da=4 will cause problems when PayPal switches to voice. Can this be removed? @fynngodau |
…esponses—for example, with PayPal.
@DaVinci9196 I tried with just the code fragment from #2898 (comment) in a minimal demo app on multiple devices, including P60 Pro with EMUI 14 and Mate X5 with Harmony OS 4 and it worked as expected and reopened the session on all of them. What device did you use for this? Looking at the code, I believe the issue might be a concurrency issue. Note that |
I have modified it according to this comment #2898 (comment), you can test it with the latest code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor nits to be fixed.
vending-app/src/main/kotlin/com/android/vending/installer/SessionResultReceiver.kt
Outdated
Show resolved
Hide resolved
vending-app/src/main/kotlin/com/android/vending/installer/Install.kt
Outdated
Show resolved
Hide resolved
...ing-app/src/main/kotlin/com/google/android/finsky/splitinstallservice/SplitInstallManager.kt
Outdated
Show resolved
Hide resolved
vending-app/src/main/kotlin/com/android/vending/installer/Install.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvements to this PR so far. However, it does not fully work yet from my tests.
vending-app/src/main/kotlin/com/android/vending/installer/Install.kt
Outdated
Show resolved
Hide resolved
vending-app/src/main/kotlin/com/android/vending/installer/Install.kt
Outdated
Show resolved
Hide resolved
vending-app/src/main/kotlin/com/android/vending/installer/Install.kt
Outdated
Show resolved
Hide resolved
vending-app/src/main/kotlin/com/android/vending/installer/Install.kt
Outdated
Show resolved
Hide resolved
My answer is: nobody really knows what these options mean. I see no regressions caused by removing it so I consider it ok. |
0.3.9 here we come! |
LGTM, Thanks @DaVinci9196 and @fynngodau for the work! |
while (inputStream.read(buffer).also { bytesRead = it } != -1) { | ||
total += bytesRead | ||
} | ||
inputStream.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mar-v-in
Wouldn't make sense to put inputStream.close()
inside a finally
block to be sure it is always closed even in case of unknown errors.
Extract the optimization and resubmit it to #2799