Swiftdiff options
| author | Tobias Markmann <tm@ayena.de> | 2015-02-07 23:48:10 (GMT) |
|---|---|---|
| committer | Swift Review <review@swift.im> | 2015-02-09 11:09:23 (GMT) |
| commit | 7d3df55bf5fd93b3eaca36cc43cb22ea7879bf7e (patch) | |
| tree | 6e48bd5233498068609f4c3e62eda25a2acfe4d6 | |
| parent | 2b81680fc59dd3170635967948727b4dc49b951e (diff) | |
| download | swift-7d3df55bf5fd93b3eaca36cc43cb22ea7879bf7e.zip swift-7d3df55bf5fd93b3eaca36cc43cb22ea7879bf7e.tar.bz2 | |
Fix data race in DummyEventLoop and BoostConnection(Server)Test reported by TSAN
The data race is on the events_ member in DummyEventLoop.
A BoostIOServerThread can post events to the DummyEventLoop and thereby
access its events_ data member while the test's main code processes
events of the loop.
To prevent access to the DummyEventLoop by the BoostIOServiceThread
after the DummyEventLoop is deleted, the BoostIOServiceThread is
deleted before the DummyEventLoop.
Process remaining events in BoostConnectionTest::tearDown like we do in
BoostConnectionServerTest::tearDown.
Test-Information:
Run multiple times on NetworkTest as TSAN enabled build on Linux.
Without the patch it reports a data race for the events_ deque member.
Change-Id: I3c85535338fc0ce0263dbfc3534aceb1dd09c137
| -rw-r--r-- | Swiften/EventLoop/DummyEventLoop.cpp | 3 | ||||
| -rw-r--r-- | Swiften/EventLoop/DummyEventLoop.h | 27 | ||||
| -rw-r--r-- | Swiften/QA/NetworkTest/BoostConnectionServerTest.cpp | 6 | ||||
| -rw-r--r-- | Swiften/QA/NetworkTest/BoostConnectionTest.cpp | 9 |
4 files changed, 35 insertions, 10 deletions
diff --git a/Swiften/EventLoop/DummyEventLoop.cpp b/Swiften/EventLoop/DummyEventLoop.cpp index 4ea425e..b8e631e 100644 --- a/Swiften/EventLoop/DummyEventLoop.cpp +++ b/Swiften/EventLoop/DummyEventLoop.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. */ @@ -14,6 +14,7 @@ DummyEventLoop::DummyEventLoop() { } DummyEventLoop::~DummyEventLoop() { + boost::lock_guard<boost::mutex> lock(eventsMutex_); if (!events_.empty()) { std::cerr << "DummyEventLoop: Unhandled events at destruction time" << std::endl; } diff --git a/Swiften/EventLoop/DummyEventLoop.h b/Swiften/EventLoop/DummyEventLoop.h index 6ee1b77..297549e 100644 --- a/Swiften/EventLoop/DummyEventLoop.h +++ b/Swiften/EventLoop/DummyEventLoop.h @@ -8,6 +8,9 @@ #include <deque> +#include <boost/thread/mutex.hpp> +#include <boost/thread/locks.hpp> + #include <Swiften/Base/API.h> #include <Swiften/EventLoop/EventLoop.h> @@ -18,21 +21,39 @@ namespace Swift { virtual ~DummyEventLoop(); void processEvents() { - while (!events_.empty()) { - handleEvent(events_[0]); - events_.pop_front(); + 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<boost::mutex> lock(eventsMutex_); + events_.pop_front(); + } } } bool hasEvents() { + boost::lock_guard<boost::mutex> lock(eventsMutex_); return !events_.empty(); } virtual void post(const Event& event) { + boost::lock_guard<boost::mutex> lock(eventsMutex_); events_.push_back(event); } private: + boost::mutex eventsMutex_; std::deque<Event> events_; }; } diff --git a/Swiften/QA/NetworkTest/BoostConnectionServerTest.cpp b/Swiften/QA/NetworkTest/BoostConnectionServerTest.cpp index 1b21042..7488d50 100644 --- a/Swiften/QA/NetworkTest/BoostConnectionServerTest.cpp +++ b/Swiften/QA/NetworkTest/BoostConnectionServerTest.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. */ @@ -24,18 +24,18 @@ class BoostConnectionServerTest : public CppUnit::TestFixture { public: void setUp() { - boostIOServiceThread_ = new BoostIOServiceThread(); eventLoop_ = new DummyEventLoop(); + boostIOServiceThread_ = new BoostIOServiceThread(); stopped = false; stoppedError.reset(); } void tearDown() { + delete boostIOServiceThread_; while (eventLoop_->hasEvents()) { eventLoop_->processEvents(); } delete eventLoop_; - delete boostIOServiceThread_; } void testConstructor_TwoServersOnSamePort() { diff --git a/Swiften/QA/NetworkTest/BoostConnectionTest.cpp b/Swiften/QA/NetworkTest/BoostConnectionTest.cpp index ff9ab0a..0cb38c3 100644 --- a/Swiften/QA/NetworkTest/BoostConnectionTest.cpp +++ b/Swiften/QA/NetworkTest/BoostConnectionTest.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. */ @@ -35,16 +35,19 @@ class BoostConnectionTest : public CppUnit::TestFixture { public: void setUp() { + eventLoop_ = new DummyEventLoop(); boostIOServiceThread_ = new BoostIOServiceThread(); boostIOService = boost::make_shared<boost::asio::io_service>(); - eventLoop_ = new DummyEventLoop(); disconnected = false; connectFinished = false; } void tearDown() { - delete eventLoop_; delete boostIOServiceThread_; + while (eventLoop_->hasEvents()) { + eventLoop_->processEvents(); + } + delete eventLoop_; } void testDestructor() { |