[core] consider offline links as finished #4394
Conversation
|
|
Does that really need fixing? I guess I could remove the spaces :P |
mihawk90
force-pushed
the
finish-offline-links
branch
from
52a1bfd
to
9c5f754
Compare
|
Fixed with a Multi-Line string. |
This change considers (permanently) offline links as finished. Although they are not technically finished in being downloaded, they also never will be so there is no reason not to dispatch `package_finished`. This allows plugins and scripts to further process the files. Note that this does not adjust the progress display in queue, allowing users to check why their progress never finishes and decide what to do with the package or links in question. Fixes pyload#4392
|
Sorry to be a party pooper but I think the PR is unneeded as you could use the |
|
I'll have to say I'm a little confused here. By "use the I don't really see a benefit in not seeing offline links as finished. What purpose does it serve to count them as "unfinished" when they will never be finished to begin with? |
|
First of all, all the events are first available to plugins, the ExternalScripts.py plugin is responsible to pass those events to scripts. Whether offline should be considered as success, this is a really good question which I'm not sure what is the correct answer for it, maybe you're right (we have to investigate what is the impact of this beside for archive extraction) I think a more elegant solution would be to hook the package_processed event and within it, do a check of the status of each files and decide if to do extraction or not. What do you think? |
Hm that's certainly possible, but I feel like that's a lot of work for little benefit. I mean realistically what are the differences between the two? Although I do think that these are 2 separate (although connected) issues:
Also it just came to me that I didn't actually test how archive extraction and possible deletion works. I just assumed the case of a failed archive extraction is handled already and it wouldn't delete the files and just leave them in that case, however I didn't check this so I'm not sure how that works. The way this PR works would be an issue if files were deleted despite the extraction failing. I think at the end of the day it comes down to what people are using the So this is the way I thought it would work. Say we have a file list of 4 files: What happens with this PR is once the first 3 files are finished it fires the finished event. For me that just unpacks, for others that might cause some scripts to run. Either way it processes those 3 files, Archive extraction would of course fail for file2 since the last part is missing, while the files for file1 would be deleted. If we processed every link individually, what does that get us? First off we'd have to track by filename what files belong together. I know this is already somewhat done for the file deletion option, but I haven't looked into how it collects those (might just be from unrar's output? I don't know). I looked into how the extraction currently works a bit more: So currently we run It just seems this information isn't processed at all, even though the error is actually going to If I'm reading this right (sorry I'm rusty with Python), the pyload/src/pyload/plugins/extractors/UnRar.py Lines 289 to 298 in 9c5f754 It parses So, if we could catch this we don't need to do a whole lot of work checking links individually. |
|
I actually just went through with a debugger and turns out I threw in a 2-liner into if err:
raise ArchiveError(err)And as expected it stops the extraction step alltogether, plus logs it to the log-output, so that is probably the most elegant way to do it. Should I add this to this PR? Since it's not directly related to how offline links are handled (but sort of related). |
|
Sorry, the first comment got a little longer then intended 👀 But TLDR while the archive extraction was my initial motivation, IMO offline links should always be treated as finished because they will never progress beyond the initial status. But you're right, needs some investigation what other potential impact it might have. That said, I did check before submitting the PR and pyload/src/pyload/core/managers/file_manager.py Lines 596 to 607 in a65a968 However I did not check what exactly it does in the addon manager. |
I was referring to something like this: def package_processed(self, pypack):
processed_successfully = all(
fdata.get("status") in (0, 1, 4)
for fid, fdata in pypack.get_children().items()
)
if processed_successfully:
self.queue.add(pypack.id)
if not self.config.get("waitall") and not self.extracting:
self.extract_queued() |
|
That would work purely for extracting sure. Either way, I dug a little to see what this would actually entail. Which then in turns calls every registered/activated plugins' pyload/src/pyload/core/managers/addon_manager.py Lines 227 to 233 in a65a968 Now when I search the code, luckily it's not super many occurrences:
pyload/src/pyload/plugins/addons/IRC.py Line 233 in a65a968
Of course it also dispatches the |
Describe the changes
This change considers (permanently) offline links as finished. Although they are not technically finished in being downloaded, they also never will be so there is no reason not to dispatch
package_finished. This allows plugins and scripts to further process the files.Note that this does not adjust the progress display in queue, allowing users to check why their progress never finishes and decide what to do with the package or links in question.
Is this related to a problem?
Fixes #4392
Additional references
Tested using links:
Fun fact: I tested with
https://raw.githubusercontent.com/pyload/pyload/main/README_NOT.mdand while that does return a 404 in the header, it's not considered offline by theDefaultPlugin, so it just downloads a text file with the error message.I didn't find any other call to
get_unfinishedso I don't think this is going to have any impact elsewhere.