Test and fix tieredIndexMock deadlocks #704
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new unit test to validate and fix deadlock issues related to tieredIndexMock by simulating thread pool execution and job processing.
- Added a guard thread to exit the process on test timeout.
- Created a LambdaJob helper to convert lambdas with captures into function pointer compatible jobs.
- Implemented multiple iterations of job submissions to validate thread pool behavior.
Comments suppressed due to low confidence (1)
tests/unit/test_common.cpp:951
- The comment indicates a 5-second timeout, but the code sets the timeout to 10 seconds. Consider updating the comment to match the actual value.
std::chrono::seconds test_timeout(10); // 5 seconds timeout for the test
rfsaliev
force-pushed
the
rfsaliev/fix-mock-tp
branch
from
05c4b6a to
96a8793
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #704 +/- ##
=======================================
Coverage 96.89% 96.89%
=======================================
Files 122 122
Lines 7350 7350
=======================================
Hits 7122 7122
Misses 228 228 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| std::generate(jobs.begin(), jobs.end(), [&]() { | ||
| return new (allocator) LambdaJob(allocator, HNSW_SEARCH_JOB, job_mock, index); | ||
| }); | ||
| mock_thread_pool.submit_callback_internal(jobs.data(), nullptr /*unused*/, |
There was a problem hiding this comment.
Can we remove the second argument from submit_callback_internal on this chance? Looks like it is redundant and a leftover
There was a problem hiding this comment.
This PR includes minimal changes to fix the deadlock issue.
But if it is needed, I can change tieredIndexMock::submit_callback_internal signature in this PR.
There was a problem hiding this comment.
not a must, we can continue as is
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* Add tieredIndexMock deadlock test * Fix tieredIndexMock deadlock * Code refine * Increase test timeout * Reduce test workload 5 times
Describe the changes in the pull request
This change includes deadlock test and fix for
tieredIndexMockused in unit tests.Which issues this PR fixes
Main objects this PR modified
Mark if applicable