From fff370c2d89acfed702b7c615650706c3c1fadd2 Mon Sep 17 00:00:00 2001 From: Jens Lorenz Date: Thu, 29 Mar 2018 16:11:34 +0200 Subject: AMUtil: Fix startup sequence addFdPoll and removeFdPoll In case plugins add new fdpoll elements and directly removing them afterwards the state inside the structure will be changed to REMOVE. REMOVE will lead all the time to an erase which will not work in case the pointer is at .end() of vector or the element itself is not the correct one. Both cases will be fixed just by checking if the fd is the same. Signed-off-by: Jens Lorenz --- AudioManagerUtilities/src/CAmSocketHandler.cpp | 16 +++-- .../AmSocketHandlerTest/CAmSocketHandlerTest.cpp | 75 ++++++++++++++++++++++ 2 files changed, 87 insertions(+), 4 deletions(-) diff --git a/AudioManagerUtilities/src/CAmSocketHandler.cpp b/AudioManagerUtilities/src/CAmSocketHandler.cpp index 1e7db6a..e5752b4 100644 --- a/AudioManagerUtilities/src/CAmSocketHandler.cpp +++ b/AudioManagerUtilities/src/CAmSocketHandler.cpp @@ -40,9 +40,9 @@ #include #endif +#define END_EVENT (UINT64_MAX >> 1) #define SHPOLL_IS_ACTIVE(state) (!(state == poll_states_e::REMOVE || state == poll_states_e::CLOSE)) - namespace am { @@ -72,7 +72,7 @@ CAmSocketHandler::CAmSocketHandler() : ssize_t bytes = read(pollfd.fd, &events, sizeof(events)); if (bytes == sizeof(events)) { - if (events == UINT64_MAX-1) + if (events >= END_EVENT) { for (auto & elem : mMapShPoll) { @@ -80,6 +80,7 @@ CAmSocketHandler::CAmSocketHandler() : elem.second.state = poll_states_e::UNINIT; } mDispatchDone = true; + } return; } @@ -145,7 +146,14 @@ void CAmSocketHandler::start_listenting() // fallthrough case poll_states_e::REMOVE: - fdPollIt = fdPollingArray.erase(fdPollIt); + /* The first check is needed for the restart behavior were an element is marked + * as removed but not part of the polling array - which is stored on heap. + * The second check is needed in case a map element was inserted newly but was + * directly marked after as REMOVE but in between there was no sync. of the + * polling array processed. + */ + if (fdPollIt != fdPollingArray.end() && fdPollIt->fd == elem.pollfdValue.fd) + fdPollIt = fdPollingArray.erase(fdPollIt); it = mMapShPoll.erase(it); continue; @@ -246,7 +254,7 @@ void CAmSocketHandler::exit_mainloop() stop_listening(); //fire the ending filedescriptor - const uint64_t events = UINT64_MAX-1; + const uint64_t events = END_EVENT; if (write(mEventFd, &events, sizeof(events)) < 0) { //Failed to write to event fd... diff --git a/AudioManagerUtilities/test/AmSocketHandlerTest/CAmSocketHandlerTest.cpp b/AudioManagerUtilities/test/AmSocketHandlerTest/CAmSocketHandlerTest.cpp index 085c3b1..a31ccf4 100644 --- a/AudioManagerUtilities/test/AmSocketHandlerTest/CAmSocketHandlerTest.cpp +++ b/AudioManagerUtilities/test/AmSocketHandlerTest/CAmSocketHandlerTest.cpp @@ -30,6 +30,7 @@ #include #include #include +#include #include "CAmDltWrapper.h" #include "CAmSocketHandler.h" @@ -56,6 +57,12 @@ struct TestUserData float f; }; +struct TestStressUserData +{ + CAmSocketHandler &socket; + std::vector &handles; +}; + MockIAmSignalHandler *pMockSignalHandler = NULL; static void signalHandler(int sig, siginfo_t *siginfo, void *context) { @@ -308,6 +315,22 @@ void* threadCallbackUnixSocketAndTimers(void* data) return sendTestData(sock, (struct sockaddr*)&servAddr, sizeof(servAddr), 500000); } +void* threadRaceFd(void* pData) +{ + struct TestStressUserData data = *(struct TestStressUserData*)pData; + usleep(50000); + auto elem = data.handles.begin(); + std::advance(elem, data.handles.size() / 2); + data.socket.removeFDPoll(*elem, sh_rmv_e::RMV_N_CLS); + data.handles.erase(elem); +} +void* threadEnd(void* pData) +{ + struct TestStressUserData data = *(struct TestStressUserData*)pData; + usleep(1000000); + data.socket.exit_mainloop(); +} + TEST(CAmSocketHandlerTest, stressTestUnixSocketAndTimers) { @@ -345,6 +368,58 @@ TEST(CAmSocketHandlerTest, stressTestUnixSocketAndTimers) shutdown(socket_, SHUT_RDWR); } + +TEST(CAmSocketHandlerTest, fdStressTest) +{ + CAmSocketHandler myHandler; + ASSERT_FALSE(myHandler.fatalErrorOccurred()); + + //Check unkonw systemd fd ids + sh_pollHandle_t handle; + EXPECT_EQ(myHandler.addFDPoll(100, 0, NULL, NULL, NULL, NULL, NULL, handle), E_NON_EXISTENT); + + int fd(-1); + std::vector handles(10); + for (auto& hndl : handles) + { + fd = eventfd(0, 0); + ASSERT_EQ(myHandler.addFDPoll(fd, POLL_IN, NULL, NULL, NULL, NULL, NULL, hndl), E_OK); + } + + // remove/add check + ASSERT_EQ(myHandler.addFDPoll(fd, POLL_IN, NULL, NULL, NULL, NULL, NULL, handles.back()), E_ALREADY_EXISTS); + ASSERT_EQ(myHandler.removeFDPoll(handles.back()), E_OK); + ASSERT_EQ(myHandler.addFDPoll(fd, POLL_IN, NULL, NULL, NULL, NULL, NULL, handles.back()), E_OK); + + // create a copy to check if all handles are removed + std::vector handlesCheckup(handles); + + while (handles.size()) + { + pthread_t tid1, tid2; + + // this removes an element before starting the socket handler and we + // erase the last handle + myHandler.removeFDPoll(handles.back(), sh_rmv_e::RMV_N_CLS); + handles.erase(handles.end()-1); + + TestStressUserData data = {myHandler, handles}; + pthread_create(&tid1, NULL, threadEnd, &data); + + // erase the handle in the middle + pthread_create(&tid2, NULL, threadRaceFd, &data); + + myHandler.start_listenting(); + + pthread_join(tid2, NULL); + pthread_join(tid1, NULL); + } + + // now do the check + for (auto& hndl : handlesCheckup) + EXPECT_EQ(myHandler.removeFDPoll(hndl), E_UNKNOWN) << "Handle " << hndl << " not correctly removed before"; +} + TEST(CAmSocketHandlerTest, timersOneshot) { CAmSocketHandler myHandler; -- cgit v1.2.1