From 6fa279737e3220c1a15362268aeb670c9185f0e1 Mon Sep 17 00:00:00 2001 From: Peter Kosyh Date: Sun, 9 Dec 2018 12:15:22 +0300 Subject: [PATCH] IMAP: sync fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch should fix: - deadlocks while manual syncing/shutdown; - crashes while manual syncing/shutdown. Change-Id: I0962ff5580c19cbf740d002b6f62681ed5f558ab Reviewed-on: https://review.haiku-os.org/758 Reviewed-by: Stephan Aßmus --- .../imap/IMAPConnectionWorker.cpp | 33 +++++++++++++------ .../imap/IMAPConnectionWorker.h | 3 ++ .../inbound_protocols/imap/IMAPFolder.cpp | 4 ++- .../inbound_protocols/imap/IMAPProtocol.cpp | 23 ++++++++++--- .../inbound_protocols/imap/IMAPProtocol.h | 2 +- 5 files changed, 49 insertions(+), 16 deletions(-) diff --git a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPConnectionWorker.cpp b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPConnectionWorker.cpp index ef870c4528..cebbc8ab39 100644 --- a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPConnectionWorker.cpp +++ b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPConnectionWorker.cpp @@ -618,10 +618,9 @@ void IMAPConnectionWorker::Quit() { printf("IMAP: worker %p: enqueue quit\n", this); - BAutolock locker(fLocker); + BAutolock qlocker(fQueueLocker); while (!fPendingCommands.IsEmpty()) delete(fPendingCommands.RemoveItemAt(0)); - locker.Unlock(); _EnqueueCommand(new QuitCommand()); } @@ -720,12 +719,12 @@ IMAPConnectionWorker::_Worker() status_t status = B_OK; while (!fStopped) { - BAutolock locker(fLocker); + BAutolock qlocker(fQueueLocker); if (fPendingCommands.IsEmpty()) { if (!fIdle) _Disconnect(); - locker.Unlock(); + qlocker.Unlock(); // TODO: in idle mode, we'd need to parse any incoming message here _WaitForCommands(); @@ -736,6 +735,8 @@ IMAPConnectionWorker::_Worker() if (command == NULL) continue; + qlocker.Unlock(); + BAutolock locker(fLocker); CommandDeleter deleter(*this, command); if (dynamic_cast(command) == NULL) { // do not connect on QuitCommand @@ -751,6 +752,7 @@ IMAPConnectionWorker::_Worker() if (!command->IsDone()) { deleter.Detach(); command->SetContinuation(); + locker.Unlock(); _EnqueueCommand(command); } } @@ -766,7 +768,7 @@ IMAPConnectionWorker::_Worker() status_t IMAPConnectionWorker::_EnqueueCommand(WorkerCommand* command) { - BAutolock locker(fLocker); + BAutolock qlocker(fQueueLocker); if (!fPendingCommands.AddItem(command)) { delete command; @@ -777,7 +779,7 @@ IMAPConnectionWorker::_EnqueueCommand(WorkerCommand* command) && !command->IsContinuation()) fSyncPending++; - locker.Unlock(); + qlocker.Unlock(); release_sem(fPendingCommandsSemaphore); return B_OK; } @@ -840,22 +842,33 @@ IMAPConnectionWorker::_SyncCommandDone() } +bool +IMAPConnectionWorker::_IsQuitPending() +{ + BAutolock locker(fQueueLocker); + WorkerCommand* nextCommand = fPendingCommands.ItemAt(0); + return dynamic_cast(nextCommand) != NULL; +} + + status_t IMAPConnectionWorker::_Connect() { if (fProtocol.IsConnected()) return B_OK; - status_t status; - int tries = 6; + status_t status = B_INTERRUPTED; + int tries = 10; while (tries-- > 0) { + if (_IsQuitPending()) + break; status = fProtocol.Connect(fSettings.ServerAddress(), fSettings.Username(), fSettings.Password(), fSettings.UseSSL()); if (status == B_OK) break; - // Wait for 10 seconds, and try again - snooze(10000000); + // Wait for 1 second, and try again + snooze(1000000); } // TODO: if other workers are connected, but it fails for us, we need to // remove this worker, and reduce the number of concurrent connections diff --git a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPConnectionWorker.h b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPConnectionWorker.h index a259a43520..48f0f76898 100644 --- a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPConnectionWorker.h +++ b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPConnectionWorker.h @@ -40,6 +40,7 @@ public: bool UsesIdle() const { return fIdle; } status_t Run(); + thread_id Thread() const { return fThread; } void Quit(); status_t EnqueueCheckSubscribedFolders(); @@ -65,6 +66,7 @@ private: void _SyncCommandDone(); uint32 _MessagesExist() const { return fMessagesExist; } + bool _IsQuitPending(); status_t _Connect(); void _Disconnect(); @@ -91,6 +93,7 @@ private: uint32 fMessagesExist; BLocker fLocker; + BLocker fQueueLocker; thread_id fThread; bool fMain; bool fStopped; diff --git a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPFolder.cpp b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPFolder.cpp index ec0cdfac37..ea3ab9b8c8 100644 --- a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPFolder.cpp +++ b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPFolder.cpp @@ -104,8 +104,10 @@ IMAPFolder::IMAPFolder(IMAPProtocol& protocol, const BString& mailboxName, IMAPFolder::~IMAPFolder() { - if (!fFolderStateInitialized) { + MutexLocker locker(fLock); + if (!fFolderStateInitialized && fListener != NULL) { fQuitFolderState = true; + locker.Unlock(); wait_for_thread(fReadFolderStateThread, NULL); } } diff --git a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPProtocol.cpp b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPProtocol.cpp index 2af060538f..642e48261b 100644 --- a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPProtocol.cpp +++ b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPProtocol.cpp @@ -38,8 +38,23 @@ IMAPProtocol::IMAPProtocol(const BMailAccountSettings& settings) IMAPProtocol::~IMAPProtocol() { -} + MutexLocker locker(fWorkerLock); + std::vector threads; + for (int32 i = 0; i < fWorkers.CountItems(); i++) { + threads.push_back(fWorkers.ItemAt(i)->Thread()); + fWorkers.ItemAt(i)->Quit(); + } + locker.Unlock(); + for (uint32 i = 0; i < threads.size(); i++) + wait_for_thread(threads[i], NULL); + + FolderMap::iterator iterator = fFolders.begin(); + for (; iterator != fFolders.end(); iterator++) { + IMAPFolder* folder = iterator->second; + delete folder; // to stop thread + } +} status_t IMAPProtocol::CheckSubscribedFolders(IMAP::Protocol& protocol, bool idle) @@ -70,7 +85,7 @@ IMAPProtocol::CheckSubscribedFolders(IMAP::Protocol& protocol, bool idle) if (newFolders.IsEmpty() && fWorkers.CountItems() == workersWanted) { // Nothing to do - we've already distributed everything - return B_OK; + return _EnqueueCheckMailboxes(); } // Remove mailboxes from workers @@ -187,8 +202,8 @@ IMAPProtocol::SyncMessages() worker->EnqueueCheckSubscribedFolders(); return worker->Run(); } - - return _EnqueueCheckMailboxes(); + fWorkers.ItemAt(0)->EnqueueCheckSubscribedFolders(); + return B_OK; } diff --git a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPProtocol.h b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPProtocol.h index 24bf46fc2d..472d165969 100644 --- a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPProtocol.h +++ b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPProtocol.h @@ -49,7 +49,7 @@ public: read_flags flags = B_READ); virtual void MessageReceived(BMessage* message); - const ::Settings* Settings() const { return &fSettings; }; + const ::Settings* Settings() const { return &fSettings; } protected: virtual status_t HandleFetchBody(const entry_ref& ref,