Skip to content

LockManager AST test #8724

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

Merged
merged 5 commits into from
Sep 3, 2025
Merged

LockManager AST test #8724

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0f5778f
LockManager AST test
asfernandes Sep 1, 2025
2d5a242
Checkout mutex in LockManagerTestCallbacks
asfernandes Sep 1, 2025
968e152
Remove test globalMutex - thanks Vlad for the patch
asfernandes Sep 3, 2025
a146df9
Misc
asfernandes Sep 3, 2025
d80a997
Misc
asfernandes Sep 3, 2025
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
123 changes: 123 additions & 0 deletions src/lock/tests/LockManagerTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
#include <atomic>
#include <latch>
#include <memory>
#include <mutex>
#include <unordered_map>
#include <thread>
#include <vector>
#include "../common/status.h"
Expand Down Expand Up @@ -173,6 +175,127 @@ BOOST_AUTO_TEST_CASE(LockUnlockNoWaitTest)
}


BOOST_AUTO_TEST_CASE(LockUnlockAstTest)
{
struct Lock;

struct ThreadData
{
LockManager* lockManager = nullptr;
std::mutex* globalMutex = nullptr;
std::mutex localMutex;
std::unordered_map<SLONG, Lock*> locks;
};

struct Lock
{
ThreadData* threadData = nullptr;
unsigned key = 0;
SLONG lockId = 0;
};

constexpr unsigned THREAD_COUNT = 8u;
constexpr unsigned ITERATION_COUNT = 10'000u;

ConfigFile configFile(ConfigFile::USE_TEXT, "\n");
Config config(configFile);

LockManagerTestCallbacks callbacks;
const string lockManagerId(getUniqueId().c_str());
auto lockManager = std::make_unique<LockManager>(lockManagerId, &config);

std::atomic_uint lockSuccess = 0u;
std::atomic_uint lockFail = 0u;

std::vector<std::thread> threads;
std::latch latch(THREAD_COUNT);

static const auto ast = [](void* astArg) -> int {
const auto lock = static_cast<Lock*>(astArg);
const auto threadData = lock->threadData;

std::lock_guard localMutexGuard(threadData->localMutex);

fb_assert(lock->lockId);

if (!threadData->lockManager->dequeue(lock->lockId))
fb_assert(false);

threadData->locks.erase(lock->lockId);
delete lock;

return 0;
};

std::mutex globalMutex;

for (unsigned threadNum = 0u; threadNum < THREAD_COUNT; ++threadNum)
{
threads.emplace_back([&, threadNum]() {
ThreadData threadData;
threadData.lockManager = lockManager.get();
threadData.globalMutex = &globalMutex;

FbLocalStatus statusVector;
LOCK_OWNER_T ownerId = threadNum + 1;
SLONG ownerHandle = 0;

lockManager->initializeOwner(&statusVector, ownerId, LCK_OWNER_attachment, &ownerHandle);

latch.arrive_and_wait();

for (unsigned i = 0; i < ITERATION_COUNT; ++i)
{
std::lock_guard globalMutexGuard(globalMutex);
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this the correct pattern to use the lock manager with ASTs?
Without both globalMutex and localMutex, problems appears.
I can't see why globalMutex should be necessary.
On the other hand, localMutex seems necessary by design, as enqueue method locks something but only a moment later the lock id is saved in the lock object, which is the object registered as parameter of the AST routine.

Copy link
Member

Choose a reason for hiding this comment

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

Didn't run it yet, so just a few notes after reading the code:

  • so far I also see no need in globalMutex
  • localMutex is necessary as it protect threadData internals
  • localMutex should be unlocked/locked in Callbacks::checkoutRun()

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the localMutex checkout, but globalMutex is still required to not crash.

Copy link
Member

Choose a reason for hiding this comment

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

I found two problems.

First one is the worker thread shutdown sequence:

  • locks are deleted before shutdownOwner() and leave possibility for AST's to access already freed object,
  • shutdownOwner() is called with callbacks and unlocked mutex and, most important, it waits for AST's delivery inside lock manager (here callback's mutex is used). This allows AST's to dequeue already dequeued lock request.

Correct sequence is to dequeue locks, call shutdownOwner() with mutex locked, and then delete locks finally.
Also, AST handler should know that lock is already dequeued and don't call dequeue() second time.

Second issue is related with enqueue():

  • there is possibility that AST handler will run after lock request is granted but before its id is known (assigned) to the external lock object. In our case AST handler call dequeue() with lockId == 0.

Attached patch fixed all this issues: lm_ast_test.patch

PS I've added threadId and ownerHandle to the ThreadData to make troubleshooting more easily.

Copy link
Member

@hvlad hvlad Sep 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed patch, first one missed lock->locked = true;
lm_ast_test.patch

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!
Now some doubts related to LCK interfaces.
LCK_lock internal and its usage seems to not have such treatment of check lck_id in AST or check blocking after lock is acquired.
So, how AST are not missed when engine code uses LCK_lock?

Copy link
Member

Choose a reason for hiding this comment

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

LCK is (almost) simple wrapper around lock manager API.
Similar checks and flags are implemented within the objects that own the locks. Many objects uses flags like blocking.
Race condition when lock is acquired first time and released by AST before lock_id is assigned is not common and was fixed in the past. It was with monitoring lock, iirc

std::lock_guard localMutexGuard(threadData.localMutex);

const auto lock = new Lock();
lock->threadData = &threadData;
lock->key = i;
lock->lockId = lockManager->enqueue(callbacks, &statusVector, 0,
LCK_expression, (const UCHAR*) &lock->key, sizeof(lock->key), LCK_EX,
ast, lock, 0, LCK_WAIT, ownerHandle);

if (lock->lockId)
{
threadData.locks.insert({ lock->lockId, lock });
++lockSuccess;
}
else
{
fb_assert(false);
delete lock;
++lockFail;
}
}

{ // scope
std::lock_guard globalMutexGuard(globalMutex);
std::lock_guard localMutexGuard(threadData.localMutex);

for (const auto [lockId, lock] : threadData.locks)
{
lockManager->dequeue(lockId);
delete lock;
}

threadData.locks.clear();
}

lockManager->shutdownOwner(callbacks, &ownerHandle);
});
}

for (auto& thread : threads)
thread.join();

BOOST_CHECK_EQUAL(lockFail.load(), 0u);
BOOST_CHECK_EQUAL(lockSuccess.load(), THREAD_COUNT * ITERATION_COUNT);

lockManager.reset();
}


BOOST_AUTO_TEST_SUITE_END() // LockManagerTests
BOOST_AUTO_TEST_SUITE_END() // LockManagerSuite
BOOST_AUTO_TEST_SUITE_END() // EngineSuite
Loading