From 37f85698cf1a799e6f8857928b61ef7fb2de3eba Mon Sep 17 00:00:00 2001 From: Clemens Zeidler Date: Fri, 11 Mar 2011 20:13:54 +0000 Subject: [PATCH] - Fix race condition when start watching a mailbox and directly afterwards stop watching it. A BLooper was not suitable to synchronise start and stop watching. Wait till the IDLE command is send before returning the SyncAndStartWatchingMailbox method now. That ensures that a later StopWatchingMailbox call find the maibox in an expected watching state. There is one thread (BLooper) to handle new commands and one watcher thread which is just listening at the server port for updates. The race condition occurred for example when a sync/watching and a fetch body message are send to the looper. The sync message just triggered the IDLE command in the watcher thread. In the meantime the fetch body command send a DONE command, because the IDLE command has not be send at this time the watcher keeps watching. - fix int32 -> ssize_t thanks Axel and Stippi - clean up git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@40919 a95241bf-73f2-0310-859d-f6bbb57e9c96 --- .../imap/IMAPInboundProtocol.cpp | 80 ++++++++++--------- .../imap/IMAPInboundProtocol.h | 16 +++- .../imap/IMAPRootInboundProtocol.cpp | 1 - .../imap/imap_lib/IMAPMailbox.cpp | 10 ++- .../imap/imap_lib/IMAPMailbox.h | 2 +- src/preferences/mail/DNSQuery.cpp | 18 ++--- src/preferences/mail/DNSQuery.h | 2 +- src/servers/mail/MailDaemon.cpp | 6 +- 8 files changed, 77 insertions(+), 58 deletions(-) diff --git a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPInboundProtocol.cpp b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPInboundProtocol.cpp index 0610e3e852..6a1c7768d4 100644 --- a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPInboundProtocol.cpp +++ b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPInboundProtocol.cpp @@ -77,39 +77,30 @@ DispatcherIMAPListener::FetchEnd() const uint32 kMsgStartWatching = '&StW'; -IMAPMailboxThread::IMAPMailboxThread(IMAPInboundProtocol& protocol, - IMAPMailbox& mailbox) - : - BLooper("IMAPMailboxThread"), - - fProtocol(protocol), - fIMAPMailbox(mailbox), - - fIsWatching(false) +int32 +watch_mailbox(void* data) { - + ((IMAPMailboxThread*)data)->_Watch(); + return B_OK; } -void -IMAPMailboxThread::MessageReceived(BMessage* message) +IMAPMailboxThread::IMAPMailboxThread(IMAPInboundProtocol& protocol, + IMAPMailbox& mailbox) + : + fProtocol(protocol), + fIMAPMailbox(mailbox), + + fIsWatching(false), + fThread(-1) { - status_t status = B_ERROR; + fWatchSyncSem = create_sem(0, "watch sync sem"); +} - switch (message->what) { - case kMsgStartWatching: - status = fIMAPMailbox.StartWatchingMailbox(); - if (status != B_OK) - fProtocol.Disconnect(); - fLock.Lock(); - fIsWatching = false; - fLock.Unlock(); - break; - - default: - BLooper::MessageReceived(message); - } +IMAPMailboxThread::~IMAPMailboxThread() +{ + delete_sem(fWatchSyncSem); } @@ -131,9 +122,14 @@ IMAPMailboxThread::SyncAndStartWatchingMailbox() BAutolock autolock(fLock); if (fIsWatching) return B_OK; + fThread = spawn_thread(watch_mailbox, "IMAPMailboxThread", + B_LOW_PRIORITY, this); + if (resume_thread(fThread) != B_OK) { + fThread = -1; + return B_ERROR; + } + acquire_sem(fWatchSyncSem); fIsWatching = true; - autolock.Unlock(); - PostMessage(kMsgStartWatching); } else { status_t status = fIMAPMailbox.CheckMailbox(); // if we lost connection reconnect and try again @@ -158,9 +154,23 @@ IMAPMailboxThread::StopWatchingMailbox() return status; // wait till watching stopped - const uint32 kMsgNoMeaning = '&NME'; - BMessage reply; - return BMessenger(this).SendMessage(kMsgNoMeaning, &reply); + status_t exitCode; + return wait_for_thread(fThread, &exitCode); +} + + +void +IMAPMailboxThread::_Watch() +{ + status_t status = fIMAPMailbox.StartWatchingMailbox(fWatchSyncSem); + if (status != B_OK) + fProtocol.Disconnect(); + + fLock.Lock(); + fIsWatching = false; + fLock.Unlock(); + + fThread = -1; } @@ -192,7 +202,6 @@ MailboxWatcher::StartWatching(const char* mailboxDir) } -#include void MailboxWatcher::MessageReceived(BMessage* message) { @@ -206,7 +215,6 @@ MailboxWatcher::MessageReceived(BMessage* message) break; switch (opcode) { case B_ENTRY_CREATED: - printf("entry created\n"); break; message->FindInt32("device", &ref.device); message->FindInt64("directory", &ref.directory); @@ -225,7 +233,6 @@ MailboxWatcher::MessageReceived(BMessage* message) case B_ENTRY_MOVED: { - printf("entry moved\n"); break; entry_ref from; entry_ref to; @@ -301,7 +308,6 @@ IMAPInboundProtocol::IMAPInboundProtocol(BMailAccountSettings* settings, fIMAPMailbox.SetFetchBodyLimit(bodyLimit); fIMAPMailboxThread = new IMAPMailboxThread(*this, fIMAPMailbox); - fIMAPMailboxThread->Run(); // set watch directory fINBOXWatcher = new MailboxWatcher(this); @@ -316,8 +322,7 @@ IMAPInboundProtocol::~IMAPInboundProtocol() RemoveHandler(fINBOXWatcher); delete fINBOXWatcher; - fIMAPMailboxThread->Lock(); - fIMAPMailboxThread->Quit(); + delete fIMAPMailboxThread; } @@ -456,7 +461,6 @@ IMAPInboundProtocol::UpdateSettings(const BMessage& settings) delete[] passwd; } - // restart mailbox's SyncMessages(); } diff --git a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPInboundProtocol.h b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPInboundProtocol.h index 837dcb2928..a06380c8bd 100644 --- a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPInboundProtocol.h +++ b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPInboundProtocol.h @@ -41,24 +41,34 @@ private: class IMAPInboundProtocol; + +int32 watch_mailbox(void* data); + + /*! Just wait for a IDLE (watching) IMAP response in this thread. */ -class IMAPMailboxThread : public BLooper { +class IMAPMailboxThread { public: IMAPMailboxThread(IMAPInboundProtocol& protocol, IMAPMailbox& mailbox); - - void MessageReceived(BMessage* message); + ~IMAPMailboxThread(); bool IsWatching(); status_t SyncAndStartWatchingMailbox(); status_t StopWatchingMailbox(); private: + void _Watch(); + + friend int32 watch_mailbox(void* data); + IMAPInboundProtocol& fProtocol; IMAPMailbox& fIMAPMailbox; BLocker fLock; bool fIsWatching; + + thread_id fThread; + sem_id fWatchSyncSem; }; diff --git a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPRootInboundProtocol.cpp b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPRootInboundProtocol.cpp index 9d51f85309..2f24efb528 100644 --- a/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPRootInboundProtocol.cpp +++ b/src/add-ons/mail_daemon/inbound_protocols/imap/IMAPRootInboundProtocol.cpp @@ -42,7 +42,6 @@ IMAPRootInboundProtocol::Connect(const char* server, const char* username, if (!folders[i].subscribed || folders[i].folder == "INBOX") continue; - IMAPInboundProtocol* inboundProtocol = new IMAPInboundProtocol( &fAccountSettings, folders[i].folder); inboundProtocol->SetMailNotifier(fMailNotifier->Clone()); diff --git a/src/add-ons/mail_daemon/inbound_protocols/imap/imap_lib/IMAPMailbox.cpp b/src/add-ons/mail_daemon/inbound_protocols/imap/imap_lib/IMAPMailbox.cpp index e64e5b47fd..3ad46e15ed 100644 --- a/src/add-ons/mail_daemon/inbound_protocols/imap/imap_lib/IMAPMailbox.cpp +++ b/src/add-ons/mail_daemon/inbound_protocols/imap/imap_lib/IMAPMailbox.cpp @@ -122,11 +122,11 @@ IMAPMailbox::SupportWatching() status_t -IMAPMailbox::StartWatchingMailbox() +IMAPMailbox::StartWatchingMailbox(sem_id startedSem) { - //TODO set it when we actually watching atomic_set(&fWatching, 1); + bool firstIDLE = true; // refresh every 29 min bigtime_t timeout = 1000 * 1000 * 60 * 29; // 29 min status_t status; @@ -134,8 +134,13 @@ IMAPMailbox::StartWatchingMailbox() int32 commandId = NextCommandId(); TRACE("IDLE ...\n"); status = SendCommand("IDLE", commandId); + if (firstIDLE) { + release_sem(startedSem); + firstIDLE = false; + } if (status != B_OK) break; + status = HandleResponse(commandId, timeout, false); ProcessAfterQuacks(kIMAP4ClientTimeout); @@ -157,6 +162,7 @@ IMAPMailbox::StartWatchingMailbox() if (status != B_OK) break; } + atomic_set(&fWatching, 0); return status; } diff --git a/src/add-ons/mail_daemon/inbound_protocols/imap/imap_lib/IMAPMailbox.h b/src/add-ons/mail_daemon/inbound_protocols/imap/imap_lib/IMAPMailbox.h index 50bc0d0a11..e1910f54a0 100644 --- a/src/add-ons/mail_daemon/inbound_protocols/imap/imap_lib/IMAPMailbox.h +++ b/src/add-ons/mail_daemon/inbound_protocols/imap/imap_lib/IMAPMailbox.h @@ -44,7 +44,7 @@ public: status_t Sync(); bool SupportWatching(); - status_t StartWatchingMailbox(); + status_t StartWatchingMailbox(sem_id startedSem = -1); status_t StopWatchingMailbox(); status_t CheckMailbox(); diff --git a/src/preferences/mail/DNSQuery.cpp b/src/preferences/mail/DNSQuery.cpp index fc2b2f95a0..ee31cfd13e 100644 --- a/src/preferences/mail/DNSQuery.cpp +++ b/src/preferences/mail/DNSQuery.cpp @@ -95,10 +95,10 @@ status_t BRawNetBuffer::ReadString(BString& string) { string = ""; - int32 read = _ReadStringAt(string, fReadPosition); - if (read < 0) + ssize_t bytesRead = _ReadStringAt(string, fReadPosition); + if (bytesRead < 0) return B_ERROR; - fReadPosition += read; + fReadPosition += bytesRead; return B_OK; } @@ -122,13 +122,13 @@ BRawNetBuffer::_Init(const void* buf, size_t size) } -int32 +ssize_t BRawNetBuffer::_ReadStringAt(BString& string, off_t pos) { if (pos >= fBuffer.BufferLength()) return -1; - int32 readed = 0; + ssize_t bytesRead = 0; char* buffer = (char*)fBuffer.Buffer(); buffer = &buffer[pos]; // if the string is compressed we have to follow the links to the @@ -137,17 +137,17 @@ BRawNetBuffer::_ReadStringAt(BString& string, off_t pos) if (uint8(*buffer) == 192) { // found a pointer mark buffer++; - readed++; + bytesRead++; off_t subPos = uint8(*buffer); _ReadStringAt(string, subPos); break; } string.Append(buffer, 1); buffer++; - readed++; + bytesRead++; } - readed++; - return readed; + bytesRead++; + return bytesRead; } diff --git a/src/preferences/mail/DNSQuery.h b/src/preferences/mail/DNSQuery.h index 51cf75bfb0..600b4bbd5a 100644 --- a/src/preferences/mail/DNSQuery.h +++ b/src/preferences/mail/DNSQuery.h @@ -42,7 +42,7 @@ public: private: void _Init(const void* buf, size_t size); - int32 _ReadStringAt(BString& string, off_t pos); + ssize_t _ReadStringAt(BString& string, off_t pos); off_t fWritePosition; off_t fReadPosition; diff --git a/src/servers/mail/MailDaemon.cpp b/src/servers/mail/MailDaemon.cpp index 2f2a110195..22dd336110 100644 --- a/src/servers/mail/MailDaemon.cpp +++ b/src/servers/mail/MailDaemon.cpp @@ -197,15 +197,15 @@ MailDaemonApp::RefsReceived(BMessage* message) sizeof(account)) < 0) continue; - InboundProtocolThread* protocol = _FindInboundProtocol(account); - if (!protocol) + InboundProtocolThread* protocolThread = _FindInboundProtocol(account); + if (!protocolThread) continue; BMessenger target; BMessenger* messenger = ⌖ if (message->FindMessenger("target", &target) != B_OK) messenger = NULL; - protocol->FetchBody(ref, messenger); + protocolThread->FetchBody(ref, messenger); } }