From fcf6a898de9c4f89db5de686aa203b3a52c18029 Mon Sep 17 00:00:00 2001 From: Tobias Markmann Date: Wed, 18 Nov 2015 14:51:58 +0100 Subject: Redesign event loops to be thread-safe and deterministic The new event loop design has a single event queue that is synchronized to protect against data races. The removal of events in the queue by EventOwner is deterministic. Test-Information: All unit and integration tests with TSAN and cxxflags=-DBOOST_SP_USE_PTHREADS on Debian 8.2 pass without reports. Multiple Swiften/QA/ClientTest/ClientTest runs under different CPU stress caused no TSAN reports on Debian 8.2. Swift itself only causes TSAN reports related to Qt itself, out of our control, and most likely false positives, i.e. TSAN not detecting the synchronization method inside Qt correctly. Unit tests pass without errors and successfully connected to Slimber on OS X 10.10.5. Change-Id: Ia1ed32ac2e758c5b9f86e0dac21362818740881e diff --git a/Swiften/EventLoop/Cocoa/CocoaEvent.h b/Swiften/EventLoop/Cocoa/CocoaEvent.h index dc38f7f..4453c74 100644 --- a/Swiften/EventLoop/Cocoa/CocoaEvent.h +++ b/Swiften/EventLoop/Cocoa/CocoaEvent.h @@ -9,7 +9,6 @@ #include namespace Swift { - class Event; class CocoaEventLoop; } @@ -19,14 +18,13 @@ namespace Swift { #pragma clang diagnostic ignored "-Wobjc-interface-ivars" @interface CocoaEvent : NSObject { - Swift::Event* event; Swift::CocoaEventLoop* eventLoop; } #pragma clang diagnostic pop // Takes ownership of event -- (id) initWithEvent: (Swift::Event*) e eventLoop: (Swift::CocoaEventLoop*) el; +- (id) init:(Swift::CocoaEventLoop*) el; - (void) process; - (void) dealloc; diff --git a/Swiften/EventLoop/Cocoa/CocoaEvent.mm b/Swiften/EventLoop/Cocoa/CocoaEvent.mm index 7b1b4b0..4f72c29 100644 --- a/Swiften/EventLoop/Cocoa/CocoaEvent.mm +++ b/Swiften/EventLoop/Cocoa/CocoaEvent.mm @@ -1,24 +1,27 @@ +/* + * Copyright (c) 2015 Isode Limited. + * All rights reserved. + * See the COPYING file for more information. + */ + #include -#include #include @implementation CocoaEvent -- (id) initWithEvent: (Swift::Event*) e eventLoop: (Swift::CocoaEventLoop*) el { +- (id) init:(Swift::CocoaEventLoop*) el { self = [super init]; if (self != nil) { - event = e; eventLoop = el; } return self; } - (void) process { - eventLoop->handleEvent(*event); + eventLoop->handleNextCocoaEvent(); } - (void) dealloc { - delete event; [super dealloc]; } diff --git a/Swiften/EventLoop/Cocoa/CocoaEventLoop.h b/Swiften/EventLoop/Cocoa/CocoaEventLoop.h index ee33fbb..aad6b0a 100644 --- a/Swiften/EventLoop/Cocoa/CocoaEventLoop.h +++ b/Swiften/EventLoop/Cocoa/CocoaEventLoop.h @@ -1,20 +1,28 @@ /* - * Copyright (c) 2010 Isode Limited. + * Copyright (c) 2010-2015 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ #pragma once +#include + #include namespace Swift { class CocoaEventLoop : public EventLoop { public: CocoaEventLoop(); + virtual ~CocoaEventLoop(); - virtual void post(const Event& event); + void handleNextCocoaEvent(); + + protected: + virtual void eventPosted(); - using EventLoop::handleEvent; + private: + bool isEventInCocoaEventLoop_; + boost::recursive_mutex isEventInCocoaEventLoopMutex_; }; } diff --git a/Swiften/EventLoop/Cocoa/CocoaEventLoop.mm b/Swiften/EventLoop/Cocoa/CocoaEventLoop.mm index ba73884..88da262 100644 --- a/Swiften/EventLoop/Cocoa/CocoaEventLoop.mm +++ b/Swiften/EventLoop/Cocoa/CocoaEventLoop.mm @@ -1,3 +1,9 @@ +/* + * Copyright (c) 2015 Isode Limited. + * All rights reserved. + * See the COPYING file for more information. + */ + #include #include @@ -5,17 +11,33 @@ namespace Swift { -CocoaEventLoop::CocoaEventLoop() { +CocoaEventLoop::CocoaEventLoop() : isEventInCocoaEventLoop_(false) { +} + +CocoaEventLoop::~CocoaEventLoop() { + +} + +void CocoaEventLoop::handleNextCocoaEvent() { + { + boost::recursive_mutex::scoped_lock lock(isEventInCocoaEventLoopMutex_); + isEventInCocoaEventLoop_ = false; + } + handleNextEvent(); } -void CocoaEventLoop::post(const Event& event) { - Event* eventCopy = new Event(event); - CocoaEvent* cocoaEvent = [[CocoaEvent alloc] initWithEvent: eventCopy eventLoop: this]; - [cocoaEvent +void CocoaEventLoop::eventPosted() { + boost::recursive_mutex::scoped_lock lock(isEventInCocoaEventLoopMutex_); + if (!isEventInCocoaEventLoop_) { + isEventInCocoaEventLoop_ = true; + + CocoaEvent* cocoaEvent = [[CocoaEvent alloc] init: this]; + [cocoaEvent performSelectorOnMainThread:@selector(process) withObject: nil waitUntilDone: NO]; - [cocoaEvent release]; + [cocoaEvent release]; + } } } diff --git a/Swiften/EventLoop/DummyEventLoop.cpp b/Swiften/EventLoop/DummyEventLoop.cpp index b8e631e..3675ead 100644 --- a/Swiften/EventLoop/DummyEventLoop.cpp +++ b/Swiften/EventLoop/DummyEventLoop.cpp @@ -10,16 +10,30 @@ namespace Swift { -DummyEventLoop::DummyEventLoop() { +DummyEventLoop::DummyEventLoop() : hasEvents_(false) { } DummyEventLoop::~DummyEventLoop() { - boost::lock_guard lock(eventsMutex_); - if (!events_.empty()) { + if (hasEvents()) { std::cerr << "DummyEventLoop: Unhandled events at destruction time" << std::endl; } - events_.clear(); } +void DummyEventLoop::processEvents() { + while(hasEvents()) { + hasEvents_ = false; + handleNextEvent(); + } +} + +bool DummyEventLoop::hasEvents() { + boost::lock_guard lock(hasEventsMutex_); + return hasEvents_; +} + +void DummyEventLoop::eventPosted() { + boost::lock_guard lock(hasEventsMutex_); + hasEvents_ = true; +} } diff --git a/Swiften/EventLoop/DummyEventLoop.h b/Swiften/EventLoop/DummyEventLoop.h index 297549e..b41cd09 100644 --- a/Swiften/EventLoop/DummyEventLoop.h +++ b/Swiften/EventLoop/DummyEventLoop.h @@ -8,8 +8,8 @@ #include -#include #include +#include #include #include @@ -20,40 +20,14 @@ namespace Swift { DummyEventLoop(); virtual ~DummyEventLoop(); - void processEvents() { - while (hasEvents()) { - /* - Creating a copy of the to-be-handled Event object because handling - it can result in a DummyEventLoop::post() call. - This call would also try to lock the eventsMutex_, resulting in a - deadlock. - */ - - eventsMutex_.lock(); - Event eventCopy = events_[0]; - eventsMutex_.unlock(); - - handleEvent(eventCopy); - - { - boost::lock_guard lock(eventsMutex_); - events_.pop_front(); - } - } - } - - bool hasEvents() { - boost::lock_guard lock(eventsMutex_); - return !events_.empty(); - } - - virtual void post(const Event& event) { - boost::lock_guard lock(eventsMutex_); - events_.push_back(event); - } + void processEvents(); + + bool hasEvents(); + + virtual void eventPosted(); private: - boost::mutex eventsMutex_; - std::deque events_; + bool hasEvents_; + boost::mutex hasEventsMutex_; }; } diff --git a/Swiften/EventLoop/EventLoop.cpp b/Swiften/EventLoop/EventLoop.cpp index 0b50e82..d9a9081 100644 --- a/Swiften/EventLoop/EventLoop.cpp +++ b/Swiften/EventLoop/EventLoop.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010-2013 Isode Limited. + * Copyright (c) 2010-2015 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ @@ -7,11 +7,12 @@ #include #include -#include #include + #include -#include #include +#include +#include #include #include @@ -26,68 +27,66 @@ inline void invokeCallback(const Event& event) { event.callback(); } catch (const std::exception& e) { - std::cerr << "Uncaught exception in event loop: " << e.what() << std::endl; + SWIFT_LOG(error) << "Uncaught exception in event loop: " << e.what() << std::endl; } catch (...) { - std::cerr << "Uncaught non-exception in event loop" << std::endl; + SWIFT_LOG(error) << "Uncaught non-exception in event loop" << std::endl; } } -EventLoop::EventLoop() : nextEventID_(0), handlingEvents_(false) { +EventLoop::EventLoop() : nextEventID_(0) { } EventLoop::~EventLoop() { } -void EventLoop::handleEvent(const Event& event) { - //SWIFT_LOG(debug) << "Handling event " << event.id << std::endl; - - if (handlingEvents_) { - // We're being called recursively. Push in the list of events to - // handle in the parent handleEvent() - eventsToHandle_.push_back(event); - return; - } - - bool doCallback = false; +void EventLoop::handleNextEvent() { + bool callEventPosted = false; { - boost::lock_guard lock(eventsMutex_); - std::list::iterator i = std::find(events_.begin(), events_.end(), event); - if (i != events_.end()) { - doCallback = true; - events_.erase(i); + boost::recursive_mutex::scoped_lock lock(removeEventsMutex_); + { + boost::optional nextEvent; + { + boost::recursive_mutex::scoped_lock lock(eventsMutex_); + if (!events_.empty()) { + nextEvent = events_.front(); + events_.pop_front(); + } + } + callEventPosted = !events_.empty(); + if (nextEvent) { + invokeCallback(nextEvent.get()); + } } + } - if (doCallback) { - handlingEvents_ = true; - invokeCallback(event); - - // Process events that were passed to handleEvent during the callback - // (i.e. through recursive calls of handleEvent) - while (!eventsToHandle_.empty()) { - Event nextEvent = eventsToHandle_.front(); - eventsToHandle_.pop_front(); - invokeCallback(nextEvent); - } - handlingEvents_ = false; + + if (callEventPosted) { + eventPosted(); } } void EventLoop::postEvent(boost::function callback, boost::shared_ptr owner) { Event event(owner, callback); + bool callEventPosted = false; { - boost::lock_guard lock(eventsMutex_); + boost::recursive_mutex::scoped_lock lock(eventsMutex_); + + callEventPosted = events_.empty(); + event.id = nextEventID_; nextEventID_++; events_.push_back(event); } - //SWIFT_LOG(debug) << "Posting event " << event.id << std::endl; - post(event); + if (callEventPosted) { + eventPosted(); + } } void EventLoop::removeEventsFromOwner(boost::shared_ptr owner) { - boost::lock_guard lock(eventsMutex_); - events_.remove_if(lambda::bind(&Event::owner, lambda::_1) == owner); + boost::recursive_mutex::scoped_lock removeLock(removeEventsMutex_); + boost::recursive_mutex::scoped_lock lock(eventsMutex_); + events_.remove_if(lambda::bind(&Event::owner, lambda::_1) == owner); } } diff --git a/Swiften/EventLoop/EventLoop.h b/Swiften/EventLoop/EventLoop.h index e64ff35..54e0fe7 100644 --- a/Swiften/EventLoop/EventLoop.h +++ b/Swiften/EventLoop/EventLoop.h @@ -1,15 +1,15 @@ /* - * Copyright (c) 2010 Isode Limited. + * Copyright (c) 2010-2015 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ #pragma once +#include + #include #include -#include -#include #include #include @@ -17,28 +17,49 @@ namespace Swift { class EventOwner; + /** + * The \ref EventLoop class provides the abstract interface for implementing event loops to use with Swiften. + * + * Events are added to the event queue using the \ref postEvent method and can be removed from the queue using + * the \ref removeEventsFromOwner method. + */ class SWIFTEN_API EventLoop { public: EventLoop(); virtual ~EventLoop(); + /** + * The \ref postEvent method allows events to be added to the event queue of the \ref EventLoop. + * An optional \ref EventOwner can be passed, allowing later removal of events that have not yet been + * executed using the \ref removeEventsFromOwner method. + */ void postEvent(boost::function event, boost::shared_ptr owner = boost::shared_ptr()); + + /** + * The \ref removeEventsFromOwner method removes all events from the specified \ref owner from the + * event queue. + */ void removeEventsFromOwner(boost::shared_ptr owner); protected: /** - * Reimplement this to call handleEvent(event) from the thread in which - * the event loop is residing. + * The \ref handleNextEvent method is called by an implementation of the abstract \ref EventLoop class + * at any point after the virtual \ref eventPosted method has been called. + * This method does not block, except for short-time synchronization. */ - virtual void post(const Event& event) = 0; + void handleNextEvent(); - void handleEvent(const Event& event); + /** + * The \ref eventPosted virtual method serves as notification for when events are still available in the queue. + * It is called after the first event is posted to an empty queue or after an event has been handled in + * \ref handleNextEvent and there are still remaining events in the queue. + */ + virtual void eventPosted() = 0; private: - boost::mutex eventsMutex_; unsigned int nextEventID_; std::list events_; - bool handlingEvents_; - std::deque eventsToHandle_; + boost::recursive_mutex eventsMutex_; + boost::recursive_mutex removeEventsMutex_; }; } diff --git a/Swiften/EventLoop/Qt/QtEventLoop.h b/Swiften/EventLoop/Qt/QtEventLoop.h index 0e048e5..389b0a7 100644 --- a/Swiften/EventLoop/Qt/QtEventLoop.h +++ b/Swiften/EventLoop/Qt/QtEventLoop.h @@ -6,28 +6,39 @@ #pragma once -#include -#include +#include + #include +#include +#include #include namespace Swift { class QtEventLoop : public QObject, public EventLoop { public: - QtEventLoop() {} + QtEventLoop() : isEventInQtEventLoop_(false) {} virtual ~QtEventLoop() { QCoreApplication::removePostedEvents(this); } - virtual void post(const Swift::Event& event) { - QCoreApplication::postEvent(this, new Event(event)); + protected: + virtual void eventPosted() { + boost::recursive_mutex::scoped_lock lock(isEventInQtEventLoopMutex_); + if (!isEventInQtEventLoop_) { + isEventInQtEventLoop_ = true; + QCoreApplication::postEvent(this, new Event()); + } } virtual bool event(QEvent* qevent) { Event* event = dynamic_cast(qevent); if (event) { - handleEvent(event->event_); + { + boost::recursive_mutex::scoped_lock lock(isEventInQtEventLoopMutex_); + isEventInQtEventLoop_ = false; + } + handleNextEvent(); //event->deleteLater(); FIXME: Leak? return true; } @@ -37,11 +48,12 @@ namespace Swift { private: struct Event : public QEvent { - Event(const Swift::Event& event) : - QEvent(QEvent::User), event_(event) { + Event() : + QEvent(QEvent::User) { } - - Swift::Event event_; }; + + bool isEventInQtEventLoop_; + boost::recursive_mutex isEventInQtEventLoopMutex_; }; } diff --git a/Swiften/EventLoop/SimpleEventLoop.cpp b/Swiften/EventLoop/SimpleEventLoop.cpp index 09b84dc..0bc06c9 100644 --- a/Swiften/EventLoop/SimpleEventLoop.cpp +++ b/Swiften/EventLoop/SimpleEventLoop.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010 Isode Limited. + * Copyright (c) 2010-2015 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ @@ -7,35 +7,28 @@ #include #include -#include #include - namespace Swift { -SimpleEventLoop::SimpleEventLoop() : isRunning_(true) { +SimpleEventLoop::SimpleEventLoop() : isRunning_(true), eventAvailable_(false) { } SimpleEventLoop::~SimpleEventLoop() { - if (!events_.empty()) { - std::cerr << "Warning: Pending events in SimpleEventLoop at destruction time" << std::endl; - } } void SimpleEventLoop::doRun(bool breakAfterEvents) { while (isRunning_) { - std::vector events; { - boost::unique_lock lock(eventsMutex_); - while (events_.empty()) { - eventsAvailable_.wait(lock); + boost::unique_lock lock(eventAvailableMutex_); + while (!eventAvailable_) { + eventAvailableCondition_.wait(lock); } - events.swap(events_); - } - foreach(const Event& event, events) { - handleEvent(event); + + eventAvailable_ = false; } + runOnce(); if (breakAfterEvents) { return; } @@ -43,14 +36,7 @@ void SimpleEventLoop::doRun(bool breakAfterEvents) { } void SimpleEventLoop::runOnce() { - std::vector events; - { - boost::unique_lock lock(eventsMutex_); - events.swap(events_); - } - foreach(const Event& event, events) { - handleEvent(event); - } + handleNextEvent(); } void SimpleEventLoop::stop() { @@ -61,12 +47,12 @@ void SimpleEventLoop::doStop() { isRunning_ = false; } -void SimpleEventLoop::post(const Event& event) { +void SimpleEventLoop::eventPosted() { { - boost::lock_guard lock(eventsMutex_); - events_.push_back(event); + boost::unique_lock lock(eventAvailableMutex_); + eventAvailable_ = true; } - eventsAvailable_.notify_one(); + eventAvailableCondition_.notify_one(); } diff --git a/Swiften/EventLoop/SimpleEventLoop.h b/Swiften/EventLoop/SimpleEventLoop.h index eedaf88..221591e 100644 --- a/Swiften/EventLoop/SimpleEventLoop.h +++ b/Swiften/EventLoop/SimpleEventLoop.h @@ -6,8 +6,6 @@ #pragma once -#include -#include #include #include @@ -31,8 +29,9 @@ namespace Swift { void runOnce(); void stop(); - - virtual void post(const Event& event); + + protected: + virtual void eventPosted(); private: void doRun(bool breakAfterEvents); @@ -40,8 +39,9 @@ namespace Swift { private: bool isRunning_; - std::vector events_; - boost::mutex eventsMutex_; - boost::condition_variable eventsAvailable_; + + bool eventAvailable_; + boost::mutex eventAvailableMutex_; + boost::condition_variable eventAvailableCondition_; }; } diff --git a/Swiften/EventLoop/SingleThreadedEventLoop.cpp b/Swiften/EventLoop/SingleThreadedEventLoop.cpp index c2235b1..095b962 100644 --- a/Swiften/EventLoop/SingleThreadedEventLoop.cpp +++ b/Swiften/EventLoop/SingleThreadedEventLoop.cpp @@ -15,20 +15,18 @@ namespace Swift { SingleThreadedEventLoop::SingleThreadedEventLoop() -: shouldShutDown_(false) +: shouldShutDown_(false), eventAvailable_(false) { } SingleThreadedEventLoop::~SingleThreadedEventLoop() { - if (!events_.empty()) { - std::cerr << "Warning: Pending events in SingleThreadedEventLoop at destruction time." << std::endl; - } + } void SingleThreadedEventLoop::waitForEvents() { - boost::unique_lock lock(eventsMutex_); - while (events_.empty() && !shouldShutDown_) { - eventsAvailable_.wait(lock); + boost::unique_lock lock(eventAvailableMutex_); + while (!eventAvailable_ && !shouldShutDown_) { + eventAvailableCondition_.wait(lock); } if (shouldShutDown_) @@ -36,30 +34,23 @@ void SingleThreadedEventLoop::waitForEvents() { } void SingleThreadedEventLoop::handleEvents() { - // Make a copy of the list of events so we don't block any threads that post - // events while we process them. - std::vector events; { - boost::unique_lock lock(eventsMutex_); - events.swap(events_); - } - - // Loop through all the events and handle them - foreach(const Event& event, events) { - handleEvent(event); + boost::lock_guard lock(eventAvailableMutex_); + eventAvailable_ = false; } + handleNextEvent(); } void SingleThreadedEventLoop::stop() { - boost::unique_lock lock(eventsMutex_); + boost::unique_lock lock(eventAvailableMutex_); shouldShutDown_ = true; - eventsAvailable_.notify_one(); + eventAvailableCondition_.notify_one(); } -void SingleThreadedEventLoop::post(const Event& event) { - boost::lock_guard lock(eventsMutex_); - events_.push_back(event); - eventsAvailable_.notify_one(); +void SingleThreadedEventLoop::eventPosted() { + boost::lock_guard lock(eventAvailableMutex_); + eventAvailable_ = true; + eventAvailableCondition_.notify_one(); } } // namespace Swift diff --git a/Swiften/EventLoop/SingleThreadedEventLoop.h b/Swiften/EventLoop/SingleThreadedEventLoop.h index 75ffad0..2145d652 100644 --- a/Swiften/EventLoop/SingleThreadedEventLoop.h +++ b/Swiften/EventLoop/SingleThreadedEventLoop.h @@ -39,7 +39,7 @@ namespace Swift { public: SingleThreadedEventLoop(); - ~SingleThreadedEventLoop(); + virtual ~SingleThreadedEventLoop(); // Blocks while waiting for new events and returns when new events are available. // Throws EventLoopCanceledException when the wait is canceled. @@ -47,12 +47,14 @@ namespace Swift { void handleEvents(); void stop(); - virtual void post(const Event& event); + protected: + virtual void eventPosted(); private: bool shouldShutDown_; - std::vector events_; - boost::mutex eventsMutex_; - boost::condition_variable eventsAvailable_; + + bool eventAvailable_; + boost::mutex eventAvailableMutex_; + boost::condition_variable eventAvailableCondition_; }; } -- cgit v0.10.2-6-g49f6