Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Darwin implementation of RunEventLoop to return when StopEventLoopTask is called. #7854

Merged
merged 1 commit into from
Jun 23, 2021
Merged

Fix Darwin implementation of RunEventLoop to return when StopEventLoopTask is called. #7854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions src/include/platform/PlatformManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,63 @@ class PlatformManager

typedef void (*EventHandlerFunct)(const ChipDeviceEvent * event, intptr_t arg);

/**
* InitChipStack() initializes the PlatformManager. After calling that, a
* consumer is allowed to call either StartEventLoopTask or RunEventLoop to
* process pending work. Calling both is not allowed: it must be one or the
* other.
*/
CHIP_ERROR InitChipStack();
CHIP_ERROR AddEventHandler(EventHandlerFunct handler, intptr_t arg = 0);
void RemoveEventHandler(EventHandlerFunct handler, intptr_t arg = 0);
/**
* ScheduleWork can be called after InitChipStack has been called. Calls
* that happen before either StartEventLoopTask or RunEventLoop will queue
* the work up but that work will NOT run until one of those functions is
* called.
*/
void ScheduleWork(AsyncWorkFunct workFunct, intptr_t arg = 0);
/**
* Process work items until StopEventLoopTask is called. RunEventLoop will
* not return until work item processing is stopped. Once it returns it
* guarantees that no more work items will be processed unless there's
* another call to RunEventLoop.
*
* Consumers that call RunEventLoop must not call StartEventLoopTask.
*
* Consumers that call RunEventLoop must ensure that RunEventLoop returns
* before calling Shutdown.
*/
void RunEventLoop();
/**
* Process work items until StopEventLoopTask is called.
*
* StartEventLoopTask processes items asynchronously. It can return before
* any items are processed, or after some items have been processed, or
* while an item is being processed, or even after StopEventLoopTask() has
* been called (e.g. if ScheduleWork() was called with a work item that
* calls StopEventLoopTask before StartEventLoopTask was called).
*
* Consumers that call StartEventLoopTask must not call RunEventLoop.
*
* Consumers that call StartEventLoopTask must ensure that they call
* StopEventLoopTask before calling Shutdown.
*/
CHIP_ERROR StartEventLoopTask();
/**
* Stop processing of work items by the event loop.
*
* If called from outside work item processing, StopEventLoopTask guarantees
* that any currently-executing work item completes execution and no more
* work items will run before it returns. This is generally how
* StopEventLoopTask is used in conjunction with StartEventLoopTask.
*
* If called from inside work item processing, StopEventLoopTask makes no
* guarantees about exactly when work item processing will stop. What it
* does guarantee is that if it is used this way in conjunction with
* RunEventLoop then all work item processing will stop before RunEventLoop
* returns.
*/
CHIP_ERROR StopEventLoopTask();
void LockChipStack();
bool TryLockChipStack();
Expand Down
39 changes: 29 additions & 10 deletions src/platform/Darwin/PlatformManagerImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ CHIP_ERROR PlatformManagerImpl::_InitChipStack()
err = Internal::PosixConfig::Init();
SuccessOrExit(err);

mRunLoopSem = dispatch_semaphore_create(0);

// Call _InitChipStack() on the generic implementation base class
// to finish the initialization process.
err = Internal::GenericPlatformManagerImpl<PlatformManagerImpl>::_InitChipStack();
Expand All @@ -68,26 +70,43 @@ CHIP_ERROR PlatformManagerImpl::_StartEventLoopTask()

CHIP_ERROR PlatformManagerImpl::_StopEventLoopTask()
{

if (mIsWorkQueueRunning == true)
{
mIsWorkQueueRunning = false;

// dispatch_sync is used in order to guarantee serialization of the caller with
// respect to any tasks that might already be on the queue, or running.
dispatch_sync(mWorkQueue, ^{
dispatch_suspend(mWorkQueue);
});
if (dispatch_get_current_queue() != mWorkQueue)
{
// dispatch_sync is used in order to guarantee serialization of the caller with
// respect to any tasks that might already be on the queue, or running.
dispatch_sync(mWorkQueue, ^{
dispatch_suspend(mWorkQueue);
});
}
else
{
// We are called from a task running on our work queue. Dispatch async,
// so we don't deadlock ourselves. Note that we do have to dispatch to
// guarantee that we don't signal the semaphore until we have ensured
// that no more tasks will run on the queue.
dispatch_async(mWorkQueue, ^{
dispatch_suspend(mWorkQueue);
dispatch_semaphore_signal(mRunLoopSem);
});
}
}

return CHIP_NO_ERROR;
};
}

void PlatformManagerImpl::_RunEventLoop()
{
_StartEventLoopTask();
CFRunLoopRun();
};

//
// Block on the semaphore till we're signalled to stop by
// _StopEventLoopTask()
//
dispatch_semaphore_wait(mRunLoopSem, DISPATCH_TIME_FOREVER);
}

CHIP_ERROR PlatformManagerImpl::_Shutdown()
{
Expand Down
5 changes: 4 additions & 1 deletion src/platform/Darwin/PlatformManagerImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,10 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener
static PlatformManagerImpl sInstance;

dispatch_queue_t mWorkQueue = nullptr;
bool mIsWorkQueueRunning = false;
// Semaphore used to implement blocking behavior in _RunEventLoop.
dispatch_semaphore_t mRunLoopSem;

bool mIsWorkQueueRunning = false;

inline ImplClass * Impl() { return static_cast<PlatformManagerImpl *>(this); }
};
Expand Down
103 changes: 98 additions & 5 deletions src/platform/tests/TestPlatformMgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,105 @@ using namespace chip::DeviceLayer;
// Unit tests
// =================================

static void TestPlatformMgr_Init(nlTestSuite * inSuite, void * inContext)
static void TestPlatformMgr_InitShutdown(nlTestSuite * inSuite, void * inContext)
{
CHIP_ERROR err = PlatformMgr().InitChipStack();
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

err = PlatformMgr().Shutdown();
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
}

static void TestPlatformMgr_BasicEventLoopTask(nlTestSuite * inSuite, void * inContext)
{
CHIP_ERROR err = PlatformMgr().InitChipStack();
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

err = PlatformMgr().StartEventLoopTask();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to actually prove from a testability standpoint the difference in behavior here vs. calling RunEventLoop?

i.e show that it returns, but still continues to do work on a separable execution context.

e.g

 CHIP_ERROR err = PlatformMgr().InitChipStack();
 NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

PlatformMgr().ScheduleWork(DidWorkFunc);

NL_TEST_ASSERT(inSuite, DidWorkCheck == false);

PlatformMgr().StartEventLoopTask();

// setup a cond var that gets signaled by the function above, but times out after a while

err = PlatformMgr().StopEventLoopTask();

...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we could probably do something like that. I ran out of time to add more interesting tests here this morning, but would be very much in favor of more of that happening!

NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

err = PlatformMgr().StopEventLoopTask();
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

err = PlatformMgr().Shutdown();
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
}

static bool stopRan;

static void StopTheLoop(intptr_t)
{
// Testing the return value here would involve multi-threaded access to the
// nlTestSuite, and it's not clear whether that's OK.
stopRan = true;
PlatformMgr().StopEventLoopTask();
}

static void TestPlatformMgr_BasicRunEventLoop(nlTestSuite * inSuite, void * inContext)
{
stopRan = false;

CHIP_ERROR err = PlatformMgr().InitChipStack();
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

PlatformMgr().ScheduleWork(StopTheLoop);

PlatformMgr().RunEventLoop();
NL_TEST_ASSERT(inSuite, stopRan);

err = PlatformMgr().Shutdown();
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
}

static void TestPlatformMgr_StartEventLoopTask(nlTestSuite * inSuite, void * inContext)
static bool sleepRan;

static void SleepSome(intptr_t)
{
CHIP_ERROR err = PlatformMgr().StartEventLoopTask();
sleep(1);
sleepRan = true;
}

static void TestPlatformMgr_RunEventLoopTwoTasks(nlTestSuite * inSuite, void * inContext)
{
stopRan = false;
sleepRan = false;

CHIP_ERROR err = PlatformMgr().InitChipStack();
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

PlatformMgr().ScheduleWork(SleepSome);
PlatformMgr().ScheduleWork(StopTheLoop);

PlatformMgr().RunEventLoop();
NL_TEST_ASSERT(inSuite, stopRan);
NL_TEST_ASSERT(inSuite, sleepRan);

err = PlatformMgr().Shutdown();
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
}

void StopAndSleep(intptr_t arg)
{
// Ensure that we don't proceed after stopping until the sleep is done too.
StopTheLoop(arg);
SleepSome(arg);
}

static void TestPlatformMgr_RunEventLoopStopBeforeSleep(nlTestSuite * inSuite, void * inContext)
{
stopRan = false;
sleepRan = false;

CHIP_ERROR err = PlatformMgr().InitChipStack();
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

PlatformMgr().ScheduleWork(StopAndSleep);

PlatformMgr().RunEventLoop();
NL_TEST_ASSERT(inSuite, stopRan);
NL_TEST_ASSERT(inSuite, sleepRan);

err = PlatformMgr().Shutdown();
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
}

Expand Down Expand Up @@ -92,8 +182,11 @@ static void TestPlatformMgr_AddEventHandler(nlTestSuite * inSuite, void * inCont
*/
static const nlTest sTests[] = {

NL_TEST_DEF("Test PlatformMgr::Init", TestPlatformMgr_Init),
NL_TEST_DEF("Test PlatformMgr::StartEventLoopTask", TestPlatformMgr_StartEventLoopTask),
NL_TEST_DEF("Test PlatformMgr::Init/Shutdown", TestPlatformMgr_InitShutdown),
NL_TEST_DEF("Test basic PlatformMgr::StartEventLoopTask", TestPlatformMgr_BasicEventLoopTask),
NL_TEST_DEF("Test basic PlatformMgr::RunEventLoop", TestPlatformMgr_BasicRunEventLoop),
NL_TEST_DEF("Test PlatformMgr::RunEventLoop with two tasks", TestPlatformMgr_RunEventLoopTwoTasks),
NL_TEST_DEF("Test PlatformMgr::RunEventLoop with stop before sleep", TestPlatformMgr_RunEventLoopStopBeforeSleep),
NL_TEST_DEF("Test PlatformMgr::TryLockChipStack", TestPlatformMgr_TryLockChipStack),
NL_TEST_DEF("Test PlatformMgr::AddEventHandler", TestPlatformMgr_AddEventHandler),

Expand Down