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

Add ability to change [FileMode] of the download by introducing [Dio… #2281

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

shehabmohamed0
Copy link

@shehabmohamed0 shehabmohamed0 commented Aug 14, 2024

…FileMode]

New Pull Request Checklist

  • [Yes] I have read the Documentation
  • [Yes] I have searched for a similar pull request in the project and found none
  • [Yes] I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
  • [Yes] I have added the required tests to prove the fix/feature I'm adding
  • [Yes] I have updated the documentation (if necessary)
  • [Yes] I have run the tests without failures
  • [No] I have updated the CHANGELOG.md in the corresponding package

Additional context and info (if any)

Add the ability to change the [FileMode] of the download without altering the default value, which is [FileMode.write], to make resuming downloads easier to implement by introducing [DioFileMode] and mapping it to FileMode in [DioForNative]
without breaking web_adapter

@shehabmohamed0 shehabmohamed0 requested a review from a team as a code owner August 14, 2024 01:34
@ueman
Copy link
Contributor

ueman commented Aug 14, 2024

Can you please explain how this change enables resuming downloads?
Please also add tests for both cases, so it will not get broken in the future.

@shehabmohamed0
Copy link
Author

Can you please explain how this change enables resuming downloads? Please also add tests for both cases, so it will not get broken in the future.

When downloading a file a stream is opened and a Random Access File are opened in the client side to store bytes
the current implementation to open a Random Access File is [FileMode.write] this mode creates a new file and overwrite the existing one if it already exists

changing the file access mode to [FileMode.append] or [FileMode.writeOnlyAppend] with change the behavior to write to the end of the file

So if you had already started downloading a file and for some reason the download stopped let's say a network problem and you set [deleteOnError = false], the file had already downloaded 50% and it's not deleted it exists but is not complete

You check if the file exists and get it's size then you request to download the file again by requesting a partial content setting the range header to file size, that means skip downloading the 50% you already downloaded then you append the remaining 50% to the end of the file and here it's you resumed your download with minimum ease.

@shehabmohamed0
Copy link
Author

Can you please explain how this change enables resuming downloads? Please also add tests for both cases, so it will not get broken in the future.

Ok, I will add tests.

@ueman
Copy link
Contributor

ueman commented Aug 14, 2024

I wonder whether it makes sense to automatically add range header in the case of a resuming download, that way it would work out of the box, without the user needing to implement anything. The server needs to support them though. (This is not something which necessarily needs to be added to this PR though, I guess)

@shehabmohamed0
Copy link
Author

I wonder whether it makes sense to automatically add range header in the case of a resuming download, that way it would work out of the box, without the user needing to implement anything. The server needs to support them though. (This is not something which necessarily needs to be added to this PR though, I guess)

Yes, it's a good idea. I think it's rare to find a server that doesn't support range requests.
Should I change the implementation to just a boolean parameter in [Dio.download] called resume ?
If the server doesn't support range requests don't use it

@shehabmohamed0
Copy link
Author

I wonder whether it makes sense to automatically add range header in the case of a resuming download, that way it would work out of the box, without the user needing to implement anything. The server needs to support them though. (This is not something which necessarily needs to be added to this PR though, I guess)

I added test case for the file mode append, now can you review the code again

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.

2 participants