From 697ae6ae84512a744958b24118197ec7bfdbc1f0 Mon Sep 17 00:00:00 2001 From: Edwin Mons Date: Tue, 19 Nov 2019 10:04:47 +0100 Subject: 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 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 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 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 lock(removeEventsMutex_); { - std::vector nextEvents; + boost::optional nextEvent; { - std::unique_lock lock(eventsMutex_); - for (int n = 0; ((n < eventsBatched) && !events_.empty()); n++) { - nextEvents.push_back(events_.front()); + std::unique_lock 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 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 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(); + 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) { -- cgit v0.10.2-6-g49f6