search

mhogomchungu / media-downloader

Media Downloader is a Qt/C++ front end to yt-dlp, youtube-dl, gallery-dl, lux, you-get, svtplay-dl, aria2c, wget and safari books..
GNU General Public License v2.0
1.34k stars 102 forks source link

media downloader freezes and must be terminated #324

Closed deee167 closed 8 months ago

deee167 commented 8 months ago

the app sometimes freezes and becomes unresponsive to the point it has to be terminated.

this seems to happen when there are active downloads happening and does not seem to matter if new downloads have been added via the clipboard monitor or by clicking the button in the ui to add the url from the clipboard. This is usually when you notice it has frozen but it seems to freeze even when there is no interaction with the app.

Other things of note: -the downloads that have started will finish successfully if you monitor the process. Ie. started processes will finish and terminate but the app still never comes back and becomes responsive again. Once it freezes, it's done -all downloads that have been queued up but not started will be lost and not saved when the app is restarted

mhogomchungu commented 8 months ago

This happens when using batch downloader or playlist downloader?

Can you consistently produce the problem?

mhogomchungu commented 8 months ago

Also, what engine are you using and what operating system are you using?

deee167 commented 8 months ago

yt-dlp and windows 10

mhogomchungu commented 8 months ago

https://github.com/mhogomchungu/media-downloader/releases/download/3.4.0/MediaDownloader-Oct-11-2023.git.zip

Download above zip file and in it, you will find "media-downloader.exe", replace the one you are using with this one and report if you still have those hangs.

deee167 commented 8 months ago

I am already using 3.4.0. That's the version that has the issues. I was already up to date.

mhogomchungu commented 8 months ago

The link above points to a more recent version than 3.4.0 and it exists so that you can test if the changes i made solved the problem.

deee167 commented 8 months ago

what did you change? Can you release it as the next version so others can test it as well. The issue only happens sometimes so not sure how long it will take to get some confidence it's fixed.

Was there something you saw in the code that could result in the freeze that you think is now fixed?

mhogomchungu commented 8 months ago

Typically, i prefer to make new released on the first day of a month.

This application is highly event based and how events are processed in Qt are determined by this list.

In certain places, i was not specifying a connection type and Qt was setting "Qt::AutoConnection" which resulted in "Qt::DirectConnection" connection type to be used.

The changes i made was to explicitly set "Qt::QueuedConnection" connection type to all events from outside the application that are meant to be processed by the UI thread.

deee167 commented 8 months ago

Thanks for the explanation. makes sense.

deee167 commented 8 months ago

tested with your fix and the app still freezes. this time it froze after all downloads were finished and cleared from the list (hidden). then with clipboard monitor enabled, when a url was copied. not sure if it froze when the url was added or before but the copied url didn't show in the list and the app was frozen and had to be killed.

mhogomchungu commented 8 months ago

What web browser are you using?

The problem seems to be with reading clipboard and i am not alone with application hanging when reading clipboard data.

Examples i have seen with issues are:

https://answers.microsoft.com/en-us/msoffice/forum/all/how-to-find-what-application-is-holding-onto/0abe2a2f-5332-45ae-96a7-18658791522b

https://forum.qt.io/topic/119587/is-it-safe-to-use-qclipboard-in-a-background-thread

https://bugreports.qt.io/browse/QTBUG-55442

https://bugs.kde.org/show_bug.cgi?id=360262

mhogomchungu commented 8 months ago

Use below version and see if it still hangs

https://github.com/mhogomchungu/media-downloader/releases/download/3.4.0/MediaDownloader-Oct-13-2023.git.zip

mhogomchungu commented 8 months ago

I have now enough confidence that the problem was due to clipboard functionality hanging the application and the current solution now fixes the issue.

The current solution reads contents of clipboard using a background thread and it uses Window's native API instead of Qt API.

deee167 commented 8 months ago

Application still froze with your latest fix from Oct 13th. It happened the same way as last time. Downloads finished and removed from list. Copy url to clipboard, see it doesn't get added in app. Click in app and it's unresponsive and frozen. Has to be killed.

mhogomchungu commented 8 months ago

There was a newer build on Oct 14th and i guess i removed it before you had a chance to download it.

I just posted a new build[1] and try it to see if you still have the problem. I am unable to reproduce this issue for some reason.

[1] https://github.com/mhogomchungu/media-downloader/releases/download/3.4.0/MediaDownloader-Oct-17-2023.git.zip

deee167 commented 8 months ago

Seems to be working Thanks

deee167 commented 8 months ago

There is another issue now. You seem to give up if you can't get the url from the clipboard and the url is silently not added. This is bad. If you detect the copy didn't work or you had to kill a thread that does the copying, you should get the url again until it works with a small delay.

mhogomchungu commented 8 months ago

The code that reads clipboard data is below and it shows Media Downloader does not use clipboard data if it shows up more than 10 seconds later. Perhaps i should make the timeout interval configurable but 10 seconds seems like a very long time and enough time to forget what url was copied with intent to add it to Media Downloader.

You think how long should Media Downloader wait by default for clipboard data?

https://github.com/mhogomchungu/media-downloader/blob/5f7a61d058a6951a0db8c8e4b353e026360e0068/src/tabmanager.cpp#L193-L216

deee167 commented 8 months ago

What do you mean? Are you saying you can only copy text from the clipboard and add it to media downloader once every 10 seconds? It sounds like you are saying Media Downloader doesn't use clipboard data after 10 seconds. That also doesn't make sense as you can copy clipboard urls indefinitely (which is how it should be). If you are saying that you only check the clipboard every 10 seconds, then this should be configurable or at least a lot shorter.

Why is this check even in there? I suggest you check if the new url copied is the same as the previous url copied and if it is, ignore it, or even better, check that the new url is already in the list of urls queued or already processed. If it is ignore it, otherwise add it.

Are you sure there is code in there to make sure the url is added if it's new, even if the thread would have frozen media downloader? Ie. if we run into the issue in this issue report, you need to make sure the url is still added. Ie. you try again. The url I added was not in the list yet, but it was also not added which is not ideal behaviour.

Can you please add support for automatically starting downloads when they are added up to the configured max downloads limit and automatically starting new downloads if already queued when one download finishes instead of waiting for all downloads to finish before starting another batch (https://github.com/mhogomchungu/media-downloader/issues/312).

mhogomchungu commented 8 months ago

Below is what happens:-

  1. Media Downloader is notified of presence of new clipboard text.

  2. Media Downloader starts a background thread and then read clipboard text on the started background thread.

  3. The reading of clipboard data takes as long as it will take and the thread just sits there and wait, forever if that is what it will take.

  4. When data finally arrives, Media Downloader checks how long the reading operation took and discards the result if it took more than ten seconds. I have made changes and the default cut off time time is now 30 seconds but you can make it unlimited if you want.

I find it strange you think it is totally fine to wait forever if that is how long it will take for clipboard text to show up.

deee167 commented 8 months ago

You should not wait forever, I said you should keep retrying. You should also not leave the thread running forever. That way you will end up with potentially many permanently blocked threads that eat up memory and resources.

I suggest you do the following: -start a thread as you do now to read the clipboard -if the thread is still running and didn't return in your 10 second timeout, arguably this could be shorter, then kill the thread and start another one to read the clipboard -repeat the above indefinitely (hopefully one of the threads will succeed in a reasonable amount of time). If however you see that this could result in never reading the clipboard even with new threads then you should have a limit on how many times you retry. I'm assuming that it's enough to just kill the thread and try reading again in a new thread and that will work in a reasonable amount of time.

Could you please add support for automatically starting downloads when added and when one finishes? Thanks

mhogomchungu commented 8 months ago

If reading clipboard data took longer than expected, then reading it again from a different thread has no guarantee it will not take just as long.

This clipboard functionality is complete as it is now and in a new version to be released later on today, you will get an opportunity to set an option to wait forever for clipboad data to show up with default value of 30 seconds timeout.

Could you please add support for automatically starting downloads when added and when one finishes? Thanks

The preferred way to add url to the UI from a webbrowser is through context menu and explanations on how to add Media Downloader context menu is here. Here you also see it is possible to add an entry and then start downloading it.