Skip to content

Remove continue from aria2c #11698

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Remove continue from aria2c #11698

wants to merge 5 commits into from
+8 −3

Conversation

tcely
Copy link

@tcely tcely commented Dec 1, 2024 •
edited
Loading

Description of your pull request and other information

Since aria2c was added the -c flag has been used.
This doesn't actually work, or make sense, without configuring the stream selector to use inorder instead of default for the algorithm.

Ideally, aria2c would write the needed information for resuming downloads in its own file before exiting. Otherwise, it's more reliable to download everything again.

Change the arguments to have aria2c try to resume, then overwrite a partial file if it can't resume, instead of aborting the download.

Arguments:

  • --auto-save-interval=10: save the control file for the transfer to disk faster than the default.
    Users can change this as they like without impacting the success of the transfer.
  • --remove-control-file: for resume to work, this must be false because true removes the control file before attempting to resume the transfer.
    Users can change this, but it's required to be false for continuedl to work as expected.
  • --allow-overwrite=true: Without this the transfer is aborted instead of overwriting the file.
    Users should not change this.
  • --always-resume=false: Without this transfers that cannot be resumed are aborted.
    Users should not change this.
  • --auto-file-renaming=false: Without this the output file is changed to a filename that doesn't already exist.
    Users should not change this.
  • --force-save=false: The control file should be removed (not saved) after the transfer is completed.
    Users should not change this.
Template

Before submitting a pull request make sure you have:

In order to be accepted and merged into yt-dlp each piece of code must be in public domain or released under Unlicense. Check all of the following options that apply:

  • I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

@tcely
Remove continue from aria2c
0b382a7 
This doesn't actually work, or make sense, without configuring the stream selector to use `inorder` instead of `default` for the algorithm.

Ideally, `aria2c` would write the needed information for resuming downloads in its own file before exiting.
Otherwise, it's more reliable to download everything again.
@bashonly bashonly added bug Bug that is not site-specific incomplete Further information is needed labels Dec 1, 2024
@tcely
aria2c should overwrite the output when resuming fails
208ab98 
Moved renaming and overwrite arguments up with the rest.
Both cases need these arguments.

Save the control file every 10 seconds.
Resume using the control file, or overwrite if not possible.
Remove the control file when the download is completed.

Print to stderr instead, just as curl does.
@tcely tcely marked this pull request as ready for review December 2, 2024 00:40
@bashonly bashonly removed the incomplete Further information is needed label Dec 3, 2024
@tcely
Honor continuedl with aria2c
087cb24 
--remove-control-file[=true|false]

Remove control file before download.
Using with --allow-overwrite=true,
download always starts from scratch.
pukkandan
pukkandan requested changes Dec 24, 2024
View reviewed changes
pukkandan
pukkandan reviewed Dec 24, 2024
View reviewed changes
tcely added 2 commits December 25, 2024 03:24
@tcely
Move required flags below _configuration_args()
670da95 
- Resume or overwrite the specified file
- Do not rename output to a non-existent file
- Do not abort the transfer when resume fails
- Do not preserve the control file after transfers complete
@tcely
Remove stderr changes
565999e
@tcely tcely requested a review from pukkandan December 25, 2024 08:57
@pukkandan
Copy link
Member

  • --auto-save-interval=10: save the control file for the transfer to disk faster than the default.
    Users can change this as they like without impacting the success of the transfer.

But why is the default insufficient?

  • --remove-control-file: for resume to work, this must be false because true removes the control file before attempting to resume the transfer.
    Users can change this, but it's required to be false for continuedl to work as expected.

The default is false, no? We can enforce this for continuedl only

  • --force-save=false: The control file should be removed (not saved) after the transfer is completed.
    Users should not change this.

This is also default, no? Could you explain why user shouldn't be able to change this?

@tcely
Copy link
Author

tcely commented Dec 25, 2024

--auto-save-interval=10

But why is the default insufficient?

The setting means there is no way to resume unless:

  1. the aria2c binary exits cleanly OR
  2. it has been transferring for longer than the configured value.

The value of '0' removes option 2.
After the transfer has made it past 10, I would like it to be able to resume if something killed the transfer without waiting for a clean shutdown to write a control file.
I wouldn't object to setting it to 60 either, but it should not be zero, if we want a reasonable chance to resume a transfer.

--remove-control-file=false

The default is false, no? We can enforce this for continuedl only

Based on a skim through the source, this default is unclear to me and it's not documented.
Setting it for the user depending on how continuedl is configured and allowing them to override it too seems appropriate to me.

--force-save=false

This is also default, no? Could you explain why user shouldn't be able to change this?

There is all manner of bad advice about this flag on the Internet. It's intended purpose is to keep the control file around after a BitTorrent transfer because the seeding statistics are stored in that file.

Allowing this to be set to true will only lead to extra unnecessary control files. It's not ever going to be useful for yt_dlp because there isn't a saved session file being loaded.

@bashonly bashonly added the core-triage triage requested from a core dev label Jul 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pukkandan pukkandan Awaiting requested review from pukkandan

Assignees
No one assigned
Labels
bug Bug that is not site-specific core-triage triage requested from a core dev
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tcely @pukkandan @bashonly