fix: bound AioContextPool wait and fail fast on empty pool - #1787
Open
aetosdios27 wants to merge 1 commit into
Open
fix: bound AioContextPool wait and fail fast on empty pool#1787aetosdios27 wants to merge 1 commit into
aetosdios27 wants to merge 1 commit into
Conversation
If every io_setup() call fails during construction, AioContextPool previously published an empty pool. pop() then waited on a predicate that could never become true, hanging every DiskANN load/search in the process forever with no error and no way out (destructor's notify_all() was ineffective since stop_ was not part of the wait predicate, and was also written without holding the mutex). Fail fast instead: throw if construction ends with zero usable contexts, include stop_ in pop()'s wait predicate so shutdown can free a blocked waiter, and guard stop_ with the mutex. Fixes zilliztech#1772 Signed-off-by: aetosdios27 <aetosdios27@gmail.com>
Collaborator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aetosdios27 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Collaborator
|
Welcome @aetosdios27! It looks like this is your first PR to zilliztech/knowhere 🎉 |
Author
|
/kind bug |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
issue: #1772
What
AioContextPool's constructor now throwsdiskann::ANNExceptionif everyio_setup()call fails, instead of publishing a pool with zero usable contexts.pop()'s wait predicate now includesstop_, so the destructor'snotify_all()can actually wake and release a caller blocked waiting for a context.stop_ = truein the destructor is now guarded byctx_mtx_, removing the data race with the waiter's predicate check.Why
AioContextPoolswallowed everyio_setup()failure and could publish an empty pool.pop()then waited onctx_q_.size()forever — a predicate that can never become true once no context was ever successfully allocated, since the only producer (push()) is reachable exclusively via a context obtained frompop()in the first place. Every DiskANN load/search in the process then hung permanently, with no error, no timeout, and — becausestop_wasn't part of the wait predicate — no way to unblock even at shutdown.This is reachable in practice whenever the process's AIO budget (
fs.aio-max-nr) is exhausted by a concurrent AIO user, which the default pool sizing makes easy to hit (512 contexts × 128 events == 65536, the defaultfs.aio-max-nr).Fail-fast turns this into a deterministic, reportable error instead of a silent hang.
AioContextPool::GetGlobalAioPool()'s constructor is only ever first invoked fromLinuxAlignedFileReader's constructor insideIndexNode::Deserialize(), which is only ever called fromIndex<T>::Deserialize()— wrapped inGuardedCall, which catchesstd::exception(ANNExceptionderives fromstd::runtime_error) and converts it toStatus::knowhere_inner_error. So the new throw cannot escape anoexceptboundary; it surfaces as a normal load failure.Not included in this PR, on purpose:
wait_fortimeout inpop()— a legitimately busy-but-healthy pool (all contexts checked out under real concurrency) would start failing spuriously. Fail-fast at construction already makes the reported repro fail deterministically without this.512 × 128 == 65536) so it no longer consumes the entire defaultfs.aio-max-nr— that looks like the actual root cause of field triggers, but it's a wider behavior change than a bug fix and deserves its own PR.Validation
pop()/destructor logic into an isolated harness withio_setup()/io_destroy()mocked out (no conan/faiss/folly/grpc needed). Confirms: (1) all-io_setup()-failures now throws instead of publishing an empty pool, (2) apop()blocked on an empty queue is released withnullptrwhen the pool is destroyed, (3) normal push/pop is unaffected.ctx_q_.size()withoutstop_) and destructor (unguardedstop_ = true) — reproduces the reported hang exactly: the whole process fails to exit and has to be killed bytimeout, matching "destroying acondition_variablewith a waiter still blocked is UB and the destructor may not return." The fixed version completes the same test in <1s.pre-commit run— no-op;thirdparty/is excluded from formatting (.pre-commit-config.yaml:20).make WITH_UT=Trueproject build and the issue's exact repro (holdingfs.aio-max-nrvia the C helper, thenIndexing.SearchDiskAnnWithInvalidParam) — not completed; the from-scratch conan build (faiss/folly/grpc/openblas) didn't finish in a reasonable time in my sandbox. Traced every call site by hand instead to confirm the new throw can't escape anoexceptboundary (caught byGuardedCallinIndex<T>::Deserialize). Recommend running this against CI or a local dev build before merge.