Swift
summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorEdwin Mons <edwin.mons@isode.com>2019-11-19 09:04:47 (GMT)
committerEdwin Mons <edwin.mons@isode.com>2019-11-19 10:51:10 (GMT)
commit697ae6ae84512a744958b24118197ec7bfdbc1f0 (patch)
treea68474dea94266efc70b710fa02d5ce491b9045c
parent8230b23238b4d0ef0fcde01a799758558d502fa1 (diff)
downloadswift-697ae6ae84512a744958b24118197ec7bfdbc1f0.zip
swift-697ae6ae84512a744958b24118197ec7bfdbc1f0.tar.bz2
Let handleNextEvent only handle a single event
A batching mechanism was added to EventLoop::handleNextEvent, which caused it to be renamed to handleNextEvents. The problem with the batching was that it breaks EventLoop::removeEventsFromOwner: events already grabbed off the events_ queue for invocation could be removed, leading to issues in cases where two events were grabbed off the queue that referred to the same entity, the second event was a timer event, and the first event caused the timer to be stopped. The timer event would in this case be executed, leading to unexpected behaviour or crashes, as shown by the added unit test. Test-Information: Unit tests pass on Debian 9 and macOS 10.14. Benchmarked the eventloop on Debian and macOS, and did not notice a performance degradation. Transferred files using S5B and IBB, and checked there were no UI hangs. Transfer speed before and after the change are roughly the same. Change-Id: Ife7312f533e8f0976c2e8077d16e0b63fbac6eb1
Diffstat
-rw-r--r--Swiften/EventLoop/BoostASIOEventLoop.cpp4
-rw-r--r--Swiften/EventLoop/Cocoa/CocoaEventLoop.mm4
-rw-r--r--Swiften/EventLoop/DummyEventLoop.cpp4
-rw-r--r--Swiften/EventLoop/EventLoop.cpp21
-rw-r--r--Swiften/EventLoop/EventLoop.h11
-rw-r--r--Swiften/EventLoop/Qt/QtEventLoop.h4
-rw-r--r--Swiften/EventLoop/SimpleEventLoop.cpp4
-rw-r--r--Swiften/EventLoop/SingleThreadedEventLoop.cpp4
-rw-r--r--Swiften/EventLoop/UnitTest/EventLoopTest.cpp13
9 files changed, 39 insertions, 30 deletions
diff --git a/Swiften/EventLoop/BoostASIOEventLoop.cpp b/Swiften/EventLoop/BoostASIOEventLoop.cpp
index 30143b9..45dd4a2 100644
--- a/Swiften/EventLoop/BoostASIOEventLoop.cpp
+++ b/Swiften/EventLoop/BoostASIOEventLoop.cpp
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2015-2016 Isode Limited.
+ * Copyright (c) 2015-2019 Isode Limited.
* All rights reserved.
* See the COPYING file for more information.
*/
@@ -23,7 +23,7 @@ void BoostASIOEventLoop::handleASIOEvent() {
std::unique_lock<std::recursive_mutex> lock(isEventInASIOEventLoopMutex_);
isEventInASIOEventLoop_ = false;
}
- handleNextEvents();
+ handleNextEvent();
}
void BoostASIOEventLoop::eventPosted() {
diff --git a/Swiften/EventLoop/Cocoa/CocoaEventLoop.mm b/Swiften/EventLoop/Cocoa/CocoaEventLoop.mm
index b8ab621..39dc7ec 100644
--- a/Swiften/EventLoop/Cocoa/CocoaEventLoop.mm
+++ b/Swiften/EventLoop/Cocoa/CocoaEventLoop.mm
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2015-2016 Isode Limited.
+ * Copyright (c) 2015-2019 Isode Limited.
* All rights reserved.
* See the COPYING file for more information.
*/
@@ -23,7 +23,7 @@ void CocoaEventLoop::handleNextCocoaEvent() {
std::unique_lock<std::recursive_mutex> lock(isEventInCocoaEventLoopMutex_);
isEventInCocoaEventLoop_ = false;
}
- handleNextEvents();
+ handleNextEvent();
}
void CocoaEventLoop::eventPosted() {
diff --git a/Swiften/EventLoop/DummyEventLoop.cpp b/Swiften/EventLoop/DummyEventLoop.cpp
index 4dfbac3..4712fad 100644
--- a/Swiften/EventLoop/DummyEventLoop.cpp
+++ b/Swiften/EventLoop/DummyEventLoop.cpp
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2010-2016 Isode Limited.
+ * Copyright (c) 2010-2019 Isode Limited.
* All rights reserved.
* See the COPYING file for more information.
*/
@@ -22,7 +22,7 @@ DummyEventLoop::~DummyEventLoop() {
void DummyEventLoop::processEvents() {
while(hasEvents()) {
hasEvents_ = false;
- handleNextEvents();
+ handleNextEvent();
}
}
diff --git a/Swiften/EventLoop/EventLoop.cpp b/Swiften/EventLoop/EventLoop.cpp
index f6af699..31c93e9 100644
--- a/Swiften/EventLoop/EventLoop.cpp
+++ b/Swiften/EventLoop/EventLoop.cpp
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2010-2018 Isode Limited.
+ * Copyright (c) 2010-2019 Isode Limited.
* All rights reserved.
* See the COPYING file for more information.
*/
@@ -35,9 +35,8 @@ EventLoop::EventLoop() : nextEventID_(0), handlingEvents_(false) {
EventLoop::~EventLoop() {
}
-void EventLoop::handleNextEvents() {
- const int eventsBatched = 100;
- // If handleNextEvents is already in progress, e.g. in case of a recursive call due to
+void EventLoop::handleNextEvent() {
+ // If handleNextEvent is already in progress, e.g. in case of a recursive call due to
// the event loop implementation, then do no handle further events. Instead call
// eventPosted() to continue event handling later.
bool callEventPosted = handlingEvents_;
@@ -45,19 +44,17 @@ void EventLoop::handleNextEvents() {
handlingEvents_ = true;
std::unique_lock<std::recursive_mutex> lock(removeEventsMutex_);
{
- std::vector<Event> nextEvents;
+ boost::optional<Event> nextEvent;
{
- std::unique_lock<std::recursive_mutex> lock(eventsMutex_);
- for (int n = 0; ((n < eventsBatched) && !events_.empty()); n++) {
- nextEvents.push_back(events_.front());
+ std::unique_lock<std::recursive_mutex> eventsLock(eventsMutex_);
+ if (!events_.empty()) {
+ nextEvent = events_.front();
events_.pop_front();
}
callEventPosted = !events_.empty();
}
- if (!nextEvents.empty()) {
- for (const auto& event : nextEvents) {
- invokeCallback(event);
- }
+ if (nextEvent) {
+ invokeCallback(*nextEvent);
}
}
handlingEvents_ = false;
diff --git a/Swiften/EventLoop/EventLoop.h b/Swiften/EventLoop/EventLoop.h
index 06b9fbb..f61b9bc 100644
--- a/Swiften/EventLoop/EventLoop.h
+++ b/Swiften/EventLoop/EventLoop.h
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2010-2016 Isode Limited.
+ * Copyright (c) 2010-2019 Isode Limited.
* All rights reserved.
* See the COPYING file for more information.
*/
@@ -43,21 +43,20 @@ namespace Swift {
protected:
/**
- * The \ref handleNextEvents method is called by an implementation of the abstract \ref EventLoop class
+ * 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.
- * It can process multiple events before it reutrns.
* If called recursively, the event queue is not further processed. Instead, \ref eventPosted
* is called to notify the implementing event loop of the non-empty event queue.
- * It is recommended to not call \ref handleNextEvents inside an event posted to the event loop
+ * It is recommended to not call \ref handleNextEvent inside an event posted to the event loop
* as this can lead to an infinite loop.
*/
- void handleNextEvents();
+ void handleNextEvent();
/**
* 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 handleNextEvents and there are still remaining events in the queue.
+ * \ref handleNextEvent and there are still remaining events in the queue.
*/
virtual void eventPosted() = 0;
diff --git a/Swiften/EventLoop/Qt/QtEventLoop.h b/Swiften/EventLoop/Qt/QtEventLoop.h
index b1644c2..cf374ab 100644
--- a/Swiften/EventLoop/Qt/QtEventLoop.h
+++ b/Swiften/EventLoop/Qt/QtEventLoop.h
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2010-2016 Isode Limited.
+ * Copyright (c) 2010-2019 Isode Limited.
* All rights reserved.
* See the COPYING file for more information.
*/
@@ -38,7 +38,7 @@ namespace Swift {
std::unique_lock<std::recursive_mutex> lock(isEventInQtEventLoopMutex_);
isEventInQtEventLoop_ = false;
}
- handleNextEvents();
+ handleNextEvent();
//event->deleteLater(); FIXME: Leak?
return true;
}
diff --git a/Swiften/EventLoop/SimpleEventLoop.cpp b/Swiften/EventLoop/SimpleEventLoop.cpp
index cac04e4..745fadb 100644
--- a/Swiften/EventLoop/SimpleEventLoop.cpp
+++ b/Swiften/EventLoop/SimpleEventLoop.cpp
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2010-2016 Isode Limited.
+ * Copyright (c) 2010-2019 Isode Limited.
* All rights reserved.
* See the COPYING file for more information.
*/
@@ -34,7 +34,7 @@ void SimpleEventLoop::doRun(bool breakAfterEvents) {
}
void SimpleEventLoop::runOnce() {
- handleNextEvents();
+ handleNextEvent();
}
void SimpleEventLoop::stop() {
diff --git a/Swiften/EventLoop/SingleThreadedEventLoop.cpp b/Swiften/EventLoop/SingleThreadedEventLoop.cpp
index 0542f37..89b4460 100644
--- a/Swiften/EventLoop/SingleThreadedEventLoop.cpp
+++ b/Swiften/EventLoop/SingleThreadedEventLoop.cpp
@@ -5,7 +5,7 @@
*/
/*
- * Copyright (c) 2016 Isode Limited.
+ * Copyright (c) 2016-2019 Isode Limited.
* All rights reserved.
* See the COPYING file for more information.
*/
@@ -43,7 +43,7 @@ void SingleThreadedEventLoop::handleEvents() {
std::lock_guard<std::mutex> lock(eventAvailableMutex_);
eventAvailable_ = false;
}
- handleNextEvents();
+ handleNextEvent();
}
void SingleThreadedEventLoop::stop() {
diff --git a/Swiften/EventLoop/UnitTest/EventLoopTest.cpp b/Swiften/EventLoop/UnitTest/EventLoopTest.cpp
index 00a4376..26c56d3 100644
--- a/Swiften/EventLoop/UnitTest/EventLoopTest.cpp
+++ b/Swiften/EventLoop/UnitTest/EventLoopTest.cpp
@@ -23,6 +23,7 @@ class EventLoopTest : public CppUnit::TestFixture {
CPPUNIT_TEST(testPost);
CPPUNIT_TEST(testRemove);
CPPUNIT_TEST(testHandleEvent_Recursive);
+ CPPUNIT_TEST(testHandleEvent_FirstEventRemovesSecondEvent);
CPPUNIT_TEST_SUITE_END();
public:
@@ -74,6 +75,18 @@ class EventLoopTest : public CppUnit::TestFixture {
CPPUNIT_ASSERT_EQUAL(1, events_[1]);
}
+ void testHandleEvent_FirstEventRemovesSecondEvent() {
+ DummyEventLoop testling;
+ auto eventOwner = std::make_shared<MyEventOwner>();
+ auto secondEventFired = false;
+
+ testling.postEvent([&](){ testling.removeEventsFromOwner(eventOwner); }, eventOwner);
+ testling.postEvent([&](){ secondEventFired = true; }, eventOwner);
+ testling.processEvents();
+
+ CPPUNIT_ASSERT_EQUAL(false, secondEventFired);
+ }
+
private:
struct MyEventOwner : public EventOwner {};
void logEvent(int i) {