Doris-Breakwater commented on issue #70505: URL: https://github.com/apache/doris/issues/70505#issuecomment-6818373105
The TLS isolation problem is confirmed by source inspection, and the reported mutex contention has a concrete code path. Suggested triage: BE inverted index / third-party CLucene, covering both TLS lifecycle correctness and scalability. Please open the proposed PR against `apache/doris-thirdparty`. Verified scope and mechanism: - Both the Doris `4.1.4` tag and commit [`e125ea85867`](https://github.com/apache/doris/tree/e125ea858678fd2aea5ebf179359650aa77fabb6/contrib) pin CLucene to `08d45e541fef1e48dc4ab00d2d5fde9569b49ee3`. I compared `ThreadLocal.cpp`, `_ThreadLocal.h`, `TermInfosReader.cpp`, and `_TermInfosReader.h` with CLucene commit `15836a0697eacd93c27faa8443ad37ac509ba43a`; these files are identical. This verifies the reported versions, without establishing the complete affected release range. - [`~_ThreadLocal()`](https://github.com/apache/doris-thirdparty/blob/08d45e541fef1e48dc4ab00d2d5fde9569b49ee3/src/core/CLucene/util/ThreadLocal.cpp#L155-L160) calls `UnregisterCurrentThread()`. That function holds `threadData_LOCK` while `ThreadLocals::UnregisterThread()` calls `setNull()` on every registered TLS object for the current thread. `setNull()` removes the value and invokes its deletor. Thus destroying A removes B's current-thread value even while B remains alive. This can happen even if the destroying thread never stored a value in A. B's values on other threads are not cleared by this call. See [cleanup and registry removal](https://github.com/apache/doris-thirdparty/blob/08d45e541fef1e48dc4ab00d2d5fde9569b49ee3/src/core/CLucene/util/ThreadLocal.cpp#L274-L341). - `RemoveThreadLocal(A)` separately visits every entry in the global thread registry and searches each thread's registration set while holding that mutex, including threads that never used A. Every **non-null** `set()` also takes this mutex, including replacement values; ordinary `get()` and `set(NULL)` use the object's `locals_LOCK` instead. Consequently, eviction can block TLS population, and the unintended clearing creates additional population work on subsequent lookups. See [`get()`, `setNull()`, and `set()`](https://github.com/apache/doris-thirdparty/blob/08d45e541fef1e48dc4ab00d2d5fde9569b49ee3/src/core/CLucene/util/ThreadLocal.cpp#L164-L270). - The Doris connection is present: the searcher cache retains a shared searcher, and the [builder sets `close_reader = true`](https://github.com/apache/doris/blob/4.1.4/be/src/storage/index/inverted/inverted_index_searcher.cpp#L30-L57). Final searcher destruction closes/deletes its reader; [`SegmentReader::doClose()`](https://github.com/apache/doris-thirdparty/blob/08d45e541fef1e48dc4ab00d2d5fde9569b49ee3/src/core/CLucene/index/SegmentReader.cpp#L322-L348) deletes `TermInfosReader`, whose [`enumerators` member](https://github.com/apache/doris-thirdparty/blob/08d45e541fef1e48dc4ab00d2d5fde9569b49ee3/src/core/CLucene/index/_TermInfosReader.h#L27-L34) is a `ThreadLocal`. [`getEnum()`](https://github.com/apache/doris-thirdparty/blob/08d45e541fef1e48dc4ab00d2d5fde9569b49ee3/src/core/CLucene/index/TermInfosReader.cpp#L216-L222) rebuilds a missing enumerator. Destruction can be deferred beyond cache eviction until active handles/shared references are released, and a composite reader can involve multiple segment readers. The proposed direction addresses both mechanisms, but patch review should establish these invariants: 1. Object destruction removes only that object's registrations and disposes of its values on **all** owning threads exactly once. Preserve real thread-exit cleanup and explicit early `UnregisterCurrentThread()` cleanup. Simply deleting the destructor's unregister call addresses the unrelated-value clearing but leaves the global scan. 2. Ownership handles remain valid across thread exit, object destruction, and registration removal. For genuinely O(owners) work, unlink through handles without searching every thread or leaving dangling reverse links. 3. Detach both registration directions before invoking value deletors, with a lifetime strategy for any TLS object still referenced by cleanup. Review callback reentrancy and lock ordering for both `threadData_LOCK` and `locals_LOCK`: current thread-exit cleanup invokes deletors under the global lock, and `setNull()` invokes them under the object lock. Freeing registry nodes after unlocking alone does not establish callback safety. Evidence still needed for the PR: - Publish the minimal A/B regression test, patch, and sanitizer commands/results. Include destruction on a thread that never owned A, multiple owners, `set(NULL)`/replacement, actual thread exit, and a value deletor that destroys another TLS object. Stress thread exit racing object destruction, with leak/double-delete checks and TSan where supported. Concurrent normal use of an object being destroyed must respect its existing lifetime contract. - Attach the component benchmark source and before/after results using identical optimized builds: total registered threads, owners per object, reader/segment counts, destruction rate, throughput, and latency percentiles. Hold owner count fixed while increasing unrelated registered threads to isolate the global-scan cost. Include symbolized wait stacks or a mutex contention trace showing both the `RemoveThreadLocal()` holder and `_ThreadLocal::set()` waiters. - For a Doris-level latency claim, include the BE build SHA, workload/reproduction steps, scanner concurrency, effective searcher-cache capacity and eviction/destruction rate, and a matching contention capture. These are needed to attribute and quantify query latency, not to establish the source-level A/B defect. I inspected the source; I have not run the reporter's unpublished unit tests, ASAN runs, or benchmark. The isolation defect and shared-lock mechanism are verified; the size of the query impact and the safety/performance of the proposed implementation remain unverified. After the third-party fix and tests are accepted, update the Doris CLucene pin and validate/backport to the confirmed affected maintained branches. Breakwater-GitHub-Analysis-Slot: slot_90aba081aa08 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
