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 issue when stopping downloads would crash when dl process has already been exited #38

Merged
merged 1 commit into from
Jul 2, 2021

Conversation

Gr3q
Copy link

@Gr3q Gr3q commented Jul 2, 2021

Almost there.

On my linux machine when I stop the download midway it comes up with and error that the process has already exited when it's trying to kill the process.

Now I tried to find why it's exiting before we are actually killing the process (maybe the consequence of the lines before), tried using os.kill or self._proc.kill() but they did not produce consistent results because the process exits somewhere else before.

So this is possibly a workaround, but even with a proper fix this try/catch doesn't hurt because it only checks for a case that should have been handled.

I can add some log line instead of a pass on the exception to make it obvious if it happened instead.

Edit: Lines here

self._proc.stderr.close()
makes the process terminate before we actually killing them. I don't want to move them because I won't test their behavior on Windows, it's up to you.

@sourcery-ai
Copy link

sourcery-ai bot commented Jul 2, 2021

Sourcery Code Quality Report

✅  Merging this PR will increase code quality in the affected files by 0.02%.

Quality metrics Before After Change
Complexity 14.48 🙂 14.54 🙂 0.06 👎
Method Length 64.70 🙂 64.90 🙂 0.20 👎
Working memory 8.94 🙂 8.92 🙂 -0.02 👍
Quality 57.53% 🙂 57.55% 🙂 0.02% 👍
Other metrics Before After Change
Lines 415 417 2
Changed files Quality Before Quality After Quality Change
youtube_dl_gui/downloaders.py 57.53% 🙂 57.55% 🙂 0.02% 👍

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation
youtube_dl_gui/downloaders.py extract_data 33 ⛔ 483 ⛔ 14 😞 18.46% ⛔ Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
youtube_dl_gui/downloaders.py YoutubeDLDownloader.download 14 🙂 148 😞 7 🙂 56.52% 🙂 Try splitting into smaller methods
youtube_dl_gui/downloaders.py YoutubeDLDownloader._last_data_hook 6 ⭐ 115 🙂 10 😞 61.08% 🙂 Extract out complex expressions
youtube_dl_gui/downloaders.py YoutubeDLDownloader._create_process 3 ⭐ 96 🙂 10 😞 66.56% 🙂 Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Help us improve this quality report!

@oleksis
Copy link
Owner

oleksis commented Jul 2, 2021

Thanks! I development on Windows and test the GUI on WSL2. The behaviour and the process on linux need more tests. I Adapted the changes and merge 😉

oleksis added a commit that referenced this pull request Jul 2, 2021
@oleksis
Copy link
Owner

oleksis commented Jul 2, 2021

Let me know is the changes work for your user case!

@Gr3q Gr3q deleted the fix-stop-issue-linux branch July 3, 2021 07:46
@Gr3q
Copy link
Author

Gr3q commented Jul 3, 2021

Published the -git version of the package on AUR, seems to work well now. I'll open other PRs if any other issues arise. If you make a new release I'll publish the non-git version as well for AUR (and I'll have a look at the built version of it as well, maybe I publish a -bin version too)

@oleksis
Copy link
Owner

oleksis commented Jul 3, 2021

@Gr3q Thanks very much! I do other sprint and we release new version 🙏🏾

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