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

Fix stream download release #618

Merged
merged 3 commits into from
Oct 25, 2022
Merged

Conversation

jvillafanez
Copy link
Member

Ref: https://github.com/owncloud/enterprise/issues/5372

Recent changes allowed concurrent uploads by using a curlMultiHandler. However, this is causing issues with the downloads because they aren't being streamed properly. Currently, with the curlMultiHandler, we need to download the whole file from S3 before starting sending it to the client. This is causing long delays in the client, which could hit timeouts, specially for big files.

In order to fix this issue, we'll use a different connection in order to handle the downloads. For downloads, we'll use the old streamHandler; for the rest of actions, the new curlMutilHandler. This way, the downloads will be streamed to the client as soon as possible, and we'll still have concurrent uploads.

@jvillafanez
Copy link
Member Author

@phil-davis I might need some help with the tests.
As far as I know, the failing tests aren't related to the files_primary_s3 app. They also fails with a regular 10.10.0 instance.
Test\LargeFileHelperGetFileSizeTest seems to fail due to some configuration issue, and the Tests\Core\Templates\TemplatesTest seems to be a formatting issue.

I guess we need to include some kind of compatibility for those tests so they can run with 10.10.0. The alternative is to drop the 10.10.0 pipeline, but it's probably a bad idea.

@jvillafanez
Copy link
Member Author

Errors might be caused by changes in the native libraries, which are part of the environment and not of ownCloud.

https://github.com/owncloud/core/blob/07b8298b80532ad29ebe80a86f49dc13946e05e6/lib/private/LargeFileHelper.php#L123 points to a change in the behavior of curl / php-curl which might be behind one of the errors. The old native curl version might not be available, or we might be fetching a more recent version
Something similar might have happened with the templates, where we're using libxml indirectly for testing.

Other than this scenario, it's weird there is any error in the tests with the final 10.10.0 version, specially when the failing tests are attached to core and don't belong to an app.

@jvillafanez
Copy link
Member Author

I have a new, more plausible theory for the failing tests.

The target commit / PR that changes things seems to be owncloud/core#40059 . Note that the commented failing tests are the same as what we have in this PR.
According to the description of the PR, the CI image was changed from ubuntu 18.04 to 20.04
In addition, there was an ongoing release for 10.10.0, and this PR was out of the release. This means that this PR was merged after 10.10.0.

I assume that the tests for the 10.10.0 release were done with the ubuntu 18.04 image and without the mentioned patch. That could explain why the tests passed at the time. Now that we're using the ubuntu 20.04 image, we might need the patch, but the patch was merged after the 10.10.0 released, so it's not available for the 10.10.0 code. That explains why the tests are now failing.

Taking into account that the 10.10.0 pipeline has been changed to 10.11.0 from the this app recently in master, I think the best solution is to cherry-pick that commit into the release.

@jvillafanez
Copy link
Member Author

#613 seems to be the issue

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@jnweiger jnweiger merged commit 36a0113 into release-1.4.0 Oct 25, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix_stream_download_release branch October 25, 2022 14:47
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