From 3a55fb4a66f411772339fe4e92da4fe545a66415 Mon Sep 17 00:00:00 2001 From: Robert Adam Date: Wed, 19 Aug 2020 16:33:21 +0200 Subject: [PATCH] FIX(client): Racecondition deleting ChannelListeners 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. --- src/ChannelListener.cpp | 16 ++++++++++++++++ src/ChannelListener.h | 11 +++++++++++ src/mumble/MainWindow.cpp | 3 +++ src/mumble/Messages.cpp | 17 ++++++++++++++++- 4 files changed, 46 insertions(+), 1 deletion(-) diff --git a/src/ChannelListener.cpp b/src/ChannelListener.cpp index 5bb9ee951..64d2fbc18 100644 --- a/src/ChannelListener.cpp +++ b/src/ChannelListener.cpp @@ -32,6 +32,7 @@ ChannelListener::ChannelListener() #ifdef MUMBLE , m_volumeLock() , m_listenerVolumeAdjustments() + , m_initialSyncDone(false) #endif {} @@ -125,6 +126,10 @@ QHash ChannelListener::getAllListenerLocalVolumeAdjustmentsImpl(bool return volumeMap; } } + +void ChannelListener::setInitialServerSyncDoneImpl(bool done) { + m_initialSyncDone.store(done); +} #endif void ChannelListener::clearImpl() { @@ -241,12 +246,23 @@ QHash ChannelListener::getAllListenerLocalVolumeAdjustments(bool fil return get().getAllListenerLocalVolumeAdjustmentsImpl(filter); } +void ChannelListener::setInitialServerSyncDone(bool done) { + get().setInitialServerSyncDoneImpl(done); +} + void ChannelListener::saveToDB() { if (!g.sh || g.sh->qbaDigest.isEmpty() || g.uiSession == 0) { // Can't save as we don't have enough context return; } + if (!get().m_initialSyncDone.load()) { + // If we were to save the listeners before the sync is done, we'd overwrite the list of listeners in the + // DB with an empty set, effectively erasing all set-up listeners. + qWarning("ChannelListener: Aborting save of user's listeners as initial ServerSync is not done yet"); + return; + } + // Save the currently listened channels g.db->setChannelListeners(g.sh->qbaDigest, ChannelListener::getListenedChannelsForUser(g.uiSession)); // And also the currently set volume adjustments (if they're not set to 1.0) diff --git a/src/ChannelListener.h b/src/ChannelListener.h index 39f3e29a8..c7046ea43 100644 --- a/src/ChannelListener.h +++ b/src/ChannelListener.h @@ -11,6 +11,8 @@ #include #include +#include + class User; class Channel; @@ -35,6 +37,9 @@ class ChannelListener : public QObject { /// A map between channel IDs and local volume adjustments to be made for ChannelListeners /// in that channel QHash m_listenerVolumeAdjustments; + + /// A flag indicating whether the initial synchronization with the server has finished yet + std::atomic m_initialSyncDone; #endif /// The static ChannelListener instance returned by ChannelListener::get() @@ -99,6 +104,9 @@ class ChannelListener : public QObject { /// @param filter Whether to filter out adjustments of 1 (which have no effect) /// @returns A map between channel IDs and the currently set volume adjustment QHash getAllListenerLocalVolumeAdjustmentsImpl(bool filter = false) const; + + /// @done Whether the initial synchronization with the server is done yet + void setInitialServerSyncDoneImpl(bool done); #endif /// Clears all ChannelListeners and volume adjustments @@ -227,6 +235,9 @@ class ChannelListener : public QObject { /// @returns A map between channel IDs and the currently set volume adjustment static QHash getAllListenerLocalVolumeAdjustments(bool filter = false); + /// @done Whether the initial synchronization with the server is done yet + static void setInitialServerSyncDone(bool done); + /// Saves the current ChannelListener state to the database. /// NOTE: This function may only be called from the main thread! static void saveToDB(); diff --git a/src/mumble/MainWindow.cpp b/src/mumble/MainWindow.cpp index b264dba14..56002d07d 100644 --- a/src/mumble/MainWindow.cpp +++ b/src/mumble/MainWindow.cpp @@ -834,6 +834,9 @@ void MainWindow::updateImagePath(QString filepath) const { } static void recreateServerHandler() { + // New server connection, so the sync has not happened yet + ChannelListener::setInitialServerSyncDone(false); + ServerHandlerPtr sh = g.sh; if (sh && sh->isRunning()) { g.mw->on_qaServerDisconnect_triggered(); diff --git a/src/mumble/Messages.cpp b/src/mumble/Messages.cpp index f789711fb..7ddbd799b 100644 --- a/src/mumble/Messages.cpp +++ b/src/mumble/Messages.cpp @@ -189,7 +189,15 @@ void MainWindow::msgServerSync(const MumbleProto::ServerSync &msg) { // Use the timer to execute the code in the main event loop as we have to access // the database. QTimer::singleShot(0, []() { - g.sh->startListeningToChannels(g.db->getChannelListeners(g.sh->qbaDigest)); + QList localListeners = g.db->getChannelListeners(g.sh->qbaDigest); + + if (!localListeners.isEmpty()) { + ChannelListener::setInitialServerSyncDone(false); + g.sh->startListeningToChannels(localListeners); + } else { + // If there are no listeners, then no synchronization is needed in the first place + ChannelListener::setInitialServerSyncDone(true); + } QHash volumeMap = g.db->getChannelListenerLocalVolumeAdjustments(g.sh->qbaDigest); @@ -455,6 +463,13 @@ void MainWindow::msgUserState(const MumbleProto::UserState &msg) { QString logMsg; if (pDst == pSelf) { logMsg = tr("You started listening to %1").arg(Log::formatChannel(c)); + + // Since ChannelListeners are sent out in bulks (all in a single message), the fact that we received + // a message that contains information about a ChannelListener of the local user means that we have + // succecssfully told the server that we are listening to the respective channels. Even if this message + // here has nothing to do with the actual initial synchronization, this means that we have been connected + // to the server long enough for the synchronization to be done. + ChannelListener::setInitialServerSyncDone(true); } else if (pSelf && pSelf->cChannel == c) { logMsg = tr("%1 started listening to your channel").arg(Log::formatClientUser(pDst, Log::Target)); }