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

[Downloads] Use a queued connection for the download manager everywhere #1076

Merged
merged 1 commit into from
Dec 12, 2020

Conversation

bugwelle
Copy link
Collaborator

@bugwelle bugwelle commented Dec 9, 2020

Fix #1073


I previously assumed that we had a threading issue. But this is not
the case because connect() calls methods of DownloadManager only in one
thread, DownloadManager's thread.
But I forgot that signals (e.g. "emit sigFinished") call their slots
immediately by default. This means we could end up in inconsistent
states because member variables are changed while we are still inside a
method that depends on them or has a previous state. For example:

 addDownload()
   startNextDownload();
    on finished: downloadFinished()
      emit sigDownloadFinished(elem);
        some slot calls addDownload, m_currentDownloadElement changes
      emit sigElemDownloaded(elem);
        may emit wrong element

To avoid this, I've adapted all places where a connect() to this
download manager is used. Those places now all used a queued
connection.

But to be more robust, we should refactor this class and get rid of
all member variables that may change state.
Also, this would allow parallel downloads.

I previously assumed that we had a threading issue.  But this is not
the case because connect() calls methods of DownloadManager only in one
thread, DownloadManager's thread.
But I forgot that signals (e.g. "emit sigFinished") call their slots
_immediately_ by default. This means we could end up in inconsistent
states because member variables are changed while we are still inside a
method that depends on them or has a previous state.  For example:

 addDownload()
   startNextDownload();
    on finished: downloadFinished()
      emit sigDownloadFinished(elem);
        some slot calls addDownload, m_currentDownloadElement changes
      emit sigElemDownloaded(elem);
        may emit wrong element

To avoid this, I've adapted all places where a connect() to this
download manager is used.  Those places now all used a queued
connection.

But to be more robust, we should refactor this class and get rid of
all member variables that may change state.
Also, this would allow parallel downloads.
@bugwelle
Copy link
Collaborator Author

I'm merging this. While it doesn't fix the crash, it is better code style.

@bugwelle bugwelle merged commit 4178a75 into Komet:master Dec 12, 2020
@bugwelle bugwelle deleted the downloadmanager branch December 12, 2020 11:49
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.

Segfault while loading information
1 participant