Commit 6f1ea5b1 authored by Hugo Beauzée-Luyssen's avatar Hugo Beauzée-Luyssen
Browse files

tests: samples: Simplify locking

We used to need a lock to be held before a discovery was scheduled and
at least until after the parsing was complete.
This is not the case anymore since the discoverer thread only emits a
discovery completed event upon full completion, rather than each time a
specific entry point gets discovered so we don't need to update various
states from multiple callback in order to wait for all folders to be
discovered and parsing to be completed after that.
This removes a potential lock inversion and simplifies the tests a bit
parent 4ed47750
Pipeline #126436 passed with stage
in 4 minutes and 36 seconds
......@@ -67,37 +67,30 @@ MockCallback::MockCallback()
{
}
std::unique_lock<compat::Mutex> MockCallback::lock()
bool MockCallback::waitForParsingComplete()
{
return std::unique_lock<compat::Mutex>{ m_parsingMutex };
}
bool MockCallback::waitForParsingComplete( std::unique_lock<compat::Mutex>& lock )
{
#ifdef CAN_USE_TRYLOCK
assert( m_parsingMutex.try_lock() == false );
#endif
m_done = false;
m_discoveryCompleted = false;
std::unique_lock<compat::Mutex> lock{ m_parsingMutex };
// Wait for a while, generating snapshots can be heavy...
return m_parsingCompleteVar.wait_for( lock, std::chrono::seconds{ 20 }, [this]() {
return m_done;
});
}
bool MockCallback::waitForRemovalComplete( std::unique_lock<compat::Mutex>& lock )
bool MockCallback::waitForRemovalComplete()
{
#ifdef CAN_USE_TRYLOCK
assert( m_parsingMutex.try_lock() == false );
#endif
assert( m_done == true );
assert( m_discoveryCompleted == true );
m_removalCompleted = false;
std::unique_lock<compat::Mutex> lock{ m_parsingMutex };
return m_parsingCompleteVar.wait_for( lock, std::chrono::seconds{ 20 }, [this]() {
return m_removalCompleted;
});
}
void MockCallback::reinit()
{
std::lock_guard<compat::Mutex> lock( m_parsingMutex );
m_discoveryCompleted = false;
m_done = false;
}
void MockCallback::prepareWaitForThumbnail( MediaPtr media )
{
m_thumbnailMutex.lock();
......@@ -173,25 +166,25 @@ void MockResumeCallback::onDiscoveryCompleted()
void MockResumeCallback::reinit()
{
std::lock_guard<compat::Mutex> lock( m_parsingMutex );
m_discoveryCompleted = true;
m_done = false;
}
bool MockResumeCallback::waitForDiscoveryComplete( std::unique_lock<compat::Mutex>& lock )
bool MockResumeCallback::waitForDiscoveryComplete()
{
assert( m_discoveryCompleted == false );
// Wait for a while, generating snapshots can be heavy...
std::unique_lock<compat::Mutex> lock{ m_parsingMutex };
return m_discoveryCompletedVar.wait_for( lock, std::chrono::seconds{ 20 }, [this]() {
return m_discoveryCompleted;
});
}
bool MockResumeCallback::waitForParsingComplete( std::unique_lock<compat::Mutex>& lock )
bool MockResumeCallback::waitForParsingComplete()
{
std::unique_lock<compat::Mutex> lock{ m_parsingMutex };
// Reimplement without checking for discovery complete. This class is meant to be used
// in 2 steps: waiting for discovery completed, then for parsing completed
assert( m_discoveryCompleted == true );
m_done = false;
// Wait for a while, generating snapshots can be heavy...
return m_parsingCompleteVar.wait_for( lock, std::chrono::seconds{ 20 }, [this]() {
return m_done;
......@@ -209,8 +202,6 @@ void Tests::InitTestCase( const std::string& testName )
buff[ret] = 0;
doc.Parse( buff );
m_lock = m_cb->lock();
if ( doc.HasMember( "banned" ) == true )
{
const auto& banned = doc["banned"];
......@@ -252,12 +243,6 @@ void Tests::SetUp( const std::string& testSuite, const std::string& testName )
void Tests::TearDown()
{
/*
* Ensures we release the mutex to avoid joining threads deadlocking while
* potentially invoking their final callbacks
* See #362
*/
m_lock.release();
/* Ensure we are closing our database connection before we try to delete it */
m_ml.reset();
ASSERT_TRUE( utils::fs::rmdir( m_testDir ) );
......@@ -920,12 +905,14 @@ void MockCallback::prepareForPlaylistReload()
// We need to force the discover to appear as complete, as we won't do any
// discovery for this test. Otherwise, we'd receive the parsing completed
// event and just ignore it.
std::lock_guard<compat::Mutex> lock{ m_parsingMutex };
m_discoveryCompleted = true;
m_done = false;
}
bool MockCallback::waitForPlaylistReload( std::unique_lock<compat::Mutex>& lock )
bool MockCallback::waitForPlaylistReload()
{
m_done = false;
std::unique_lock<compat::Mutex> lock{ m_parsingMutex };
// Wait for a while, generating snapshots can be heavy...
return m_parsingCompleteVar.wait_for( lock, std::chrono::seconds{ 20 }, [this]() {
return m_done;
......@@ -934,8 +921,7 @@ bool MockCallback::waitForPlaylistReload( std::unique_lock<compat::Mutex>& lock
void MockCallback::prepareForRemoval( uint32_t nbEntryPointsRemovalExpected )
{
#ifdef CAN_USE_TRYLOCK
assert( m_parsingMutex.try_lock() == false );
#endif
std::lock_guard<compat::Mutex> lock{ m_parsingMutex };
m_nbEntryPointsRemovalExpected = nbEntryPointsRemovalExpected;
m_removalCompleted = false;
}
......@@ -50,15 +50,14 @@ class MockCallback : public mock::NoopCallback
{
public:
MockCallback();
virtual bool waitForParsingComplete( std::unique_lock<compat::Mutex>& lock );
virtual bool waitForDiscoveryComplete( std::unique_lock<compat::Mutex>& ) { return true; }
virtual bool waitForRemovalComplete( std::unique_lock<compat::Mutex>& );
virtual void reinit() {}
virtual bool waitForParsingComplete();
virtual bool waitForDiscoveryComplete() { return true; }
virtual bool waitForRemovalComplete();
virtual void reinit();
void prepareWaitForThumbnail( MediaPtr media );
bool waitForThumbnail();
std::unique_lock<compat::Mutex> lock();
void prepareForPlaylistReload();
bool waitForPlaylistReload( std::unique_lock<compat::Mutex>& lock );
bool waitForPlaylistReload();
void prepareForDiscovery( uint32_t nbEntryPointsExpected );
void prepareForRemoval( uint32_t nbEntryPointsRemovalExpected );
......@@ -86,8 +85,8 @@ protected:
class MockResumeCallback : public MockCallback
{
public:
virtual bool waitForDiscoveryComplete( std::unique_lock<compat::Mutex>& lock ) override;
virtual bool waitForParsingComplete( std::unique_lock<compat::Mutex>& lock ) override;
virtual bool waitForDiscoveryComplete() override;
virtual bool waitForParsingComplete() override;
virtual void onDiscoveryCompleted() override;
virtual void reinit() override;
......@@ -103,7 +102,6 @@ struct Tests
std::unique_ptr<MockCallback> m_cb;
std::unique_ptr<IMediaLibrary> m_ml;
std::unique_lock<compat::Mutex> m_lock;
rapidjson::Document doc;
rapidjson::GenericValue<rapidjson::UTF8<>> input;
......
......@@ -36,14 +36,14 @@
static void Parse( Tests* T )
{
ASSERT_TRUE( T->m_cb->waitForParsingComplete( T->m_lock ) );
ASSERT_TRUE( T->m_cb->waitForParsingComplete() );
T->runChecks();
}
static void ParseTwice( Tests* T )
{
ASSERT_TRUE( T->m_cb->waitForParsingComplete( T->m_lock ) );
ASSERT_TRUE( T->m_cb->waitForParsingComplete() );
T->runChecks();
......@@ -55,7 +55,8 @@ static void ParseTwice( Tests* T )
T->m_ml->removeEntryPoint( utils::file::toMrl( samplesDir ) );
}
ASSERT_TRUE( T->m_cb->waitForRemovalComplete( T->m_lock ) );
ASSERT_TRUE( T->m_cb->waitForRemovalComplete() );
T->m_cb->reinit();
for ( auto i = 0u; i < T->input.Size(); ++i )
{
......@@ -64,46 +65,46 @@ static void ParseTwice( Tests* T )
T->m_ml->discover( utils::file::toMrl( samplesDir ) );
}
ASSERT_TRUE( T->m_cb->waitForParsingComplete( T->m_lock ) );
ASSERT_TRUE( T->m_cb->waitForParsingComplete() );
T->runChecks();
}
static void RunResumeTests( ResumeTests* T )
{
ASSERT_TRUE( T->m_cb->waitForDiscoveryComplete( T->m_lock ) );
ASSERT_TRUE( T->m_cb->waitForDiscoveryComplete() );
auto testMl = static_cast<MediaLibraryResumeTest*>( T->m_ml.get() );
testMl->forceParserStart();
ASSERT_TRUE( T->m_cb->waitForParsingComplete( T->m_lock ) );
ASSERT_TRUE( T->m_cb->waitForParsingComplete() );
T->runChecks();
}
static void Rescan( ResumeTests* T )
{
ASSERT_TRUE( T->m_cb->waitForDiscoveryComplete( T->m_lock ) );
ASSERT_TRUE( T->m_cb->waitForDiscoveryComplete() );
auto testMl = static_cast<MediaLibraryResumeTest*>( T->m_ml.get() );
testMl->forceParserStart();
ASSERT_TRUE( T->m_cb->waitForParsingComplete( T->m_lock ) );
ASSERT_TRUE( T->m_cb->waitForParsingComplete() );
T->m_cb->reinit();
T->m_ml->forceRescan();
ASSERT_TRUE( T->m_cb->waitForParsingComplete( T->m_lock ) );
ASSERT_TRUE( T->m_cb->waitForParsingComplete() );
T->runChecks();
}
static void RunRefreshTests( RefreshTests* T )
{
ASSERT_TRUE( T->m_cb->waitForDiscoveryComplete( T->m_lock ) );
ASSERT_TRUE( T->m_cb->waitForParsingComplete( T->m_lock ) );
ASSERT_TRUE( T->m_cb->waitForDiscoveryComplete() );
ASSERT_TRUE( T->m_cb->waitForParsingComplete() );
T->runChecks();
T->m_cb->reinit();
T->forceRefresh();
ASSERT_TRUE( T->m_cb->waitForParsingComplete( T->m_lock ) );
ASSERT_TRUE( T->m_cb->waitForParsingComplete() );
T->runChecks();
}
......@@ -114,19 +115,18 @@ static void ReplaceVlcInstance( Tests* T )
T->m_ml->setExternalLibvlcInstance( inst.get() );
/* Replacing the instance will stop the discoverer so let's resume it */
T->m_ml->reload();
ASSERT_TRUE( T->m_cb->waitForParsingComplete( T->m_lock ) );
ASSERT_TRUE( T->m_cb->waitForParsingComplete() );
T->runChecks();
}
static void RunBackupRestorePlaylist( BackupRestorePlaylistTests* T )
{
auto lock = T->m_cb->lock();
auto samplesFolder = std::string{ SRC_DIR "/test/samples/samples/playlist/tracks" };
ASSERT_TRUE( utils::fs::isDirectory( samplesFolder ) );
samplesFolder = utils::fs::toAbsolute( samplesFolder );
T->m_ml->discover( utils::file::toMrl( samplesFolder ) );
auto res = T->m_cb->waitForParsingComplete( lock );
auto res = T->m_cb->waitForParsingComplete();
ASSERT_TRUE( res );
// Now we should have discovered some media
......@@ -153,7 +153,7 @@ static void RunBackupRestorePlaylist( BackupRestorePlaylistTests* T )
T->m_cb->prepareForPlaylistReload();
T->m_ml->clearDatabase( true );
res = T->m_cb->waitForPlaylistReload( lock );
res = T->m_cb->waitForPlaylistReload();
ASSERT_TRUE( res );
auto playlists = T->m_ml->playlists( nullptr )->all();
......@@ -175,8 +175,9 @@ static void RunBackupRestorePlaylist( BackupRestorePlaylistTests* T )
* converted back to internal media, meaning they'll recover their titles
* and duration among other information.
*/
T->m_cb->reinit();
T->m_ml->discover( utils::file::toMrl( samplesFolder ) );
res = T->m_cb->waitForParsingComplete( lock );
res = T->m_cb->waitForParsingComplete();
ASSERT_TRUE( res );
media = playlist1->media( nullptr )->all();
ASSERT_EQ( m1->title(), media[0]->title() );
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment