Fix raceconditions in the client #4430
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Krzmbrzl
added a commit
to Krzmbrzl/mumble
that referenced
this pull request
Aug 19, 2020
Server-specific shortcuts are read into memory in MainWindow::msgServerSync and are saved into the DB again in MainWindow::serverDisconnected. If however the disconnect function runs before the server synchronization has finished, the shortcut list in memory is empty and by writing that into the DB all existing server-specific shortcuts are lost (overwritten). This commit adds checks before saving the shortcuts so that saving only occurs if the synchronization has happened before and thus the shortcuts have been loaded in the first place. (Backported from mumble-voip#4430 and adapted to work with the 1.3.x series)
Krzmbrzl
added a commit
to Krzmbrzl/mumble
that referenced
this pull request
Aug 19, 2020
Server-specific shortcuts are read into memory in MainWindow::msgServerSync and are saved into the DB again in MainWindow::serverDisconnected. If however the disconnect function runs before the server synchronization has finished, the shortcut list in memory is empty and by writing that into the DB all existing server-specific shortcuts are lost (overwritten). This commit adds checks before saving the shortcuts so that saving only occurs if the synchronization has happened before and thus the shortcuts have been loaded in the first place. (Backported from mumble-voip#4430 and adapted to work with the 1.3.x series)
9bbcd83 to
043eeae
Compare
davidebeatrici
approved these changes
Aug 19, 2020
Server-specific shortcuts are read into memory in MainWindow::msgServerSync and are saved into the DB again in MainWindow::serverDisconnected. If however the disconnect function runs before the server synchronization has finished, the shortcut list in memory is empty and by writing that into the DB all existing server-specific shortcuts are lost (overwritten). This commit adds checks before saving the shortcuts (and ChannelListeners) so that saving only occurs if the synchronization has happened before and thus the shortcuts have been loaded in the first place.
As ChannelListeners are only synchronised with the server once msgServerSync has been received, there is still a racecondition for them even though we only save them if the ServerSync has happened yet. The connection could be terminated before the answer of the server for the request to add the respective listeners reaches the client. In that case the listeners would be erased by the call to ChannelListener::saveToDB(). Thus this commit introduces yet another flag indicating whether the listeners have synchronised with the server yet.
The processing of ChannelListeners was delayed until the next iteration of the Qt event loop since it was thought that msgServerSync was running in another thread. Given that this function already accesses the DB, this can't be though. Thus the delay is unnecessary and was therefore removed.
043eeae to
ba06bb1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes 2 raceconditions that both occur due to the
MainWindow::serverDisconnectedbeing able to trigger while certain client-server communication is still happening (unfinished).The effects of these are
Below are some notes of what exactly is happening for 1) to happen. The cause for 2) is rather similar and should be understandable from the commit-message.