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

Windows download hang #919

Closed
wants to merge 3 commits into from
Closed

Conversation

adamlamar
Copy link
Collaborator

@adamlamar adamlamar commented Jan 25, 2023

Overall, the high level design is something like:

  • Run a new QThread using the BackgroundDownloader. Any slots invoked on the BackgroundDownloader are run on this new thread (not the UI thread)
  • The ContentManager sends signals from the UI thread to the BackgroundDownloader thread.
  • When needed, the BackgroundDownloader sends signals back to the ContentManager to confirm operations, such as starting or canceling a download
  • The BackgroundDownloader::updateStatus() is invoked once per second by a QTimer, and updates the internal m_status map with information about the download
  • The BackgroundDownloader:: getDownloadStatus() method can be called from the ContentManager on the UI thread to get the status of any particular download
  • Since BackgroundDownloader can be called from two different threads, reading from the m_status map is protected by a ReadWrite lock

Fixes #87

}

// getDownloadStatus returns the status map for this downloadId. May be called from a separate thread
QMap<std::string, std::string> BackgroundDownloader::getDownloadStatus(const std::string did)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getDownloadStatus() is called by ContentManager:: updateDownloadInfos(), which is running on the UI thread. m_rwlock protects the map read.

{
QThread* newThread = new QThread(); // no parent
m_thread.reset(newThread);
moveToThread(m_thread.get());
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parentless thread and call to moveToThread() is key to ensuring that BackgroundDownloader slots are invoked on their own thread that is separate from the UI thread.

@kelson42
Copy link
Collaborator

Hi @adamlamar. Have you given up on this PR? After the implementation of #921, nothing really blocks the completion here, do I'm right?

@kelson42
Copy link
Collaborator

kelson42 commented Oct 4, 2023

@mgautierfr Its important to finally complete this PR.

@kelson42
Copy link
Collaborator

kelson42 commented Apr 6, 2024

@adamlamar @veloman-yunkan @mgautierfr This PR which was containing quite a bit of work has been superseeded by #1038. Do we agree that this PR does not contain piece of code we should still merge to main and that we can close it unmerged?

@adamlamar
Copy link
Collaborator Author

Yep! Closing

@adamlamar adamlamar closed this Apr 6, 2024
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.

Kiwix goes into non responsive when trying to download 78GB wikipedia
2 participants