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

[5.2] Chunked download for the Joomla Update process #43489

Draft
wants to merge 18 commits into
base: 5.2-dev
Choose a base branch
from

Conversation

MacJoom
Copy link
Contributor

@MacJoom MacJoom commented May 19, 2024

Please read the instructions carefully! Needs a new Joomla Installation and access to the joomla files (no Patch Tester)

Summary of Changes

  • Introduce a new download process, first checks for the availability of the download source with a configurable timeout, gets filesize at the same time
  • Does a chunked download (disabled by default, see options in update component)
  • Fixes problem that only the first mirror was used - thanks to @Septdir
  • Logs the download source, logs chunked download

Based on the #39280 from @nikosdion - thank you!

Testing Instructions

Cannot be tested using Patch Tester as there are NPM resources changed and need the joomla package to build (use prebuilt packages)
You need access to your joomla installation (e.g. FTP)

  • Download the prebuild package under the checks below.
  • Install Joomla
  • Configure the options in the Update Component under Fine-tuning (Chunked downloads is disabled by default), the timeout in seconds for the first call to the downloadsite can be set there.
  • Test with Chunked downloads disabled and enabled, try out different chunk sizes
  • Manually modify libraries/src/Version.php: Set public const MINOR_VERSION = 1
    this way an update to 5.1.0 stable will be proposed since EXTRA_VERSION is on alpha1-dev (if not set it to alpha1-dev)
    you cannot update to 5.2.0 versions since there are no packages yet.

For experts only:
You can simulate an unavailable downloadserver with a setting in /etc/hosts of your server:
0.0.0.1 s3-us-west-2.amazonaws.com
and/or
0.0.0.1 objects.githubusercontent.com

REMEMBER: Your installation will be overwritten by the update! After the update your site will be on 5.1!

Actual result BEFORE applying this Pull Request

  • Update sometimes fails because download may timeout
  • Update sometimes fails because download fails on unavailable mirror

Expected result AFTER applying this Pull Request

  • Update works as before
  • Chunked download may fix problems with slow downloads

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.2-dev labels May 19, 2024
@MacJoom MacJoom closed this May 19, 2024
@Septdir
Copy link
Contributor

Septdir commented May 19, 2024

That restoring mirrors is not enough just removing unset.
If you want to restore their work, check out my PR.
There the very principle of working with mirrors was updated. And it’s not about timeout, of course, it’s needed to exclude non-working servers, but it doesn’t affect the very principle of receiving a link.

@MacJoom
Copy link
Contributor Author

MacJoom commented May 19, 2024

That restoring mirrors is not enough just removing unset. If you want to restore their work, check out my PR. There the very principle of working with mirrors was updated. And it’s not about timeout, of course, it’s needed to exclude non-working servers, but it doesn’t affect the very principle of receiving a link.

Ok thanks - i will look into it further. However in my tests i have all the expected download servers in the array.

@MacJoom MacJoom reopened this May 19, 2024
Thanks a lot!

Co-authored-by: Brian Teeman <brian@teeman.net>
@Septdir
Copy link
Contributor

Septdir commented May 20, 2024

That restoring mirrors is not enough just removing unset. If you want to restore their work, check out my PR. There the very principle of working with mirrors was updated. And it’s not about timeout, of course, it’s needed to exclude non-working servers, but it doesn’t affect the very principle of receiving a link.

Ok thanks - i will look into it further. However in my tests i have all the expected download servers in the array.

It's not about the availability of all the links, but about processing them.

Initially, the update, including mirrors, worked like this:
In cycles, we try to download the file from each source in turn.

Then the logic was corrected:

  1. We only get the headers to get the final file link.
  2. We try to download; if it doesn't download, we use mirrors.

https://github.com/joomla/joomla-cms/blob/4.4-dev/administrator/components/com_joomlaupdate/src/Model/UpdateModel.php#L363

Getting the final link through headers is the right approach, but it should consider mirrors exactly in the first step.

So the correct algorithm is:

  1. We only get the headers to get the final file link. If that fails, we try to get it from a mirror.
  2. We download the file from the final link.

Let me tell you about the timeout again.
When executing a PHP script, we have X seconds (depending on server settings), after which a timeout response will follow, either just a white screen, or if the server is not set up correctly, it may even result in long polling.
At the same time, the timeout of the download server may exceed X, and ultimately our update script will just fall into a timeout without ever receiving a response from the main mirror.

I took the timeout time based on standard PHP settings.

Honestly, it might have been better to first complete #43489 and adopt it to restore the update functionality (for example, the timeout could have been made a setting, and then users with slow speeds could just increase the timeout values after changing the php.ini settings), and then seriously approach the redesign.
But as they say, "It's up to you".

If you approach the update functionality intelligently, then you need to revise the principle.
For example, first, you could break the update into three separate Ajax requests:

  1. We get the headers to obtain the server's response time and the link to the file itself. On all mirrors (one by one). We give out the link with the minimum response time.
  2. We download the archive.
  3. We install the update.

If something goes wrong but the archive has been downloaded, you can simply restart step 3.

However, in the case of chunks, it's not that simple. And the main problem is this:
What if the update server stops responding, for example, on the 5th part of the file?
When I thought through the functionality of my Joomla update server, I still couldn't find an answer to this question.

I hope this will be useful, and thank you for your work.

@Septdir
Copy link
Contributor

Septdir commented May 20, 2024

Another option is to completely abandon the iteration over all mirrors and give users the choice of which mirror to download from.

For example, I could choose Github on my end, while someone else might keep Amazon. Someone else might choose the third mirror.

Such an approach would eliminate the need to iterate through mirrors and give users the option.
"If the download fails, try switching the mirror in the settings."
A list of mirrors, just a select box with preset values.

This way, you can comfortably attempt to implement chunks while reducing the script execution time.

MacJoom and others added 2 commits May 21, 2024 10:45
Thanks again

Co-authored-by: Brian Teeman <brian@teeman.net>
*
* @return void
*
* @since 2.5.4
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @since 2.5.4
* @since __DEPLOY_VERSION__

/**
* Start the installation of the new Joomla! version
*
* @return void
*
* @since 2.5.4
*/
public function install()
public function install(): void
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function install(): void
public function install()

b/c break

Copy link
Contributor

Choose a reason for hiding this comment

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

Component's not subject to b/c policy?

@MacJoom MacJoom marked this pull request as draft May 26, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.2-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants