Copilot commented on code in PR #66020: URL: https://github.com/apache/doris/pull/66020#discussion_r3645692769
########## be/test/util/bthread_shared_mutex_test.cpp: ########## @@ -0,0 +1,264 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "util/bthread_shared_mutex.h" + +#include <bthread/bthread.h> +#include <gtest/gtest.h> + +#include <atomic> +#include <shared_mutex> +#include <thread> +#include <vector> + +namespace doris { + +// 1) Basic SharedMutex contract. try_lock/try_lock_shared are non-blocking, so +// this can be checked single-threaded. +TEST(BthreadSharedMutexTest, BasicContract) { + BthreadSharedMutex m; + + // Multiple shared holders can coexist. + ASSERT_TRUE(m.try_lock_shared()); + ASSERT_TRUE(m.try_lock_shared()); + // A writer cannot acquire while any reader is held. + ASSERT_FALSE(m.try_lock()); + m.unlock_shared(); + ASSERT_FALSE(m.try_lock()); // still one reader + m.unlock_shared(); + + // Exclusive ownership. + ASSERT_TRUE(m.try_lock()); + ASSERT_FALSE(m.try_lock()); // exclusive: a second writer fails + ASSERT_FALSE(m.try_lock_shared()); // a reader is blocked while the writer holds + m.unlock(); + + // Free again. + ASSERT_TRUE(m.try_lock_shared()); + m.unlock_shared(); +} + +// 2) Writer-pending behavior of the two-gate algorithm: once a writer has set +// the write-entered bit and is waiting for existing readers to drain, new +// readers must block until the writer acquires and releases the lock. +namespace { +struct WriterPendingCtx { + BthreadSharedMutex* m; + std::atomic<bool> acquired {false}; + std::atomic<bool> released {false}; +}; + +void* writer_pending_fn(void* arg) { + auto* c = static_cast<WriterPendingCtx*>(arg); + c->m->lock(); // sets write-entered, then blocks until the reader drains + c->acquired.store(true); + bthread_usleep(20 * 1000); + c->m->unlock(); + c->released.store(true); + return nullptr; +} +} // namespace + +TEST(BthreadSharedMutexTest, WriterPendingBlocksNewReaders) { + bthread_setconcurrency(8); + BthreadSharedMutex m; + m.lock_shared(); // reader R1 holds a shared lock + + WriterPendingCtx ctx; + ctx.m = &m; + bthread_t w; + ASSERT_EQ(0, bthread_start_background(&w, nullptr, writer_pending_fn, &ctx)); + + // Wait until the writer has reached lock() and set the write-entered bit, + // observed via try_lock_shared() beginning to fail (writer-preferring: a new + // reader must not barge ahead of a pending writer). Probe instead of a fixed + // sleep so a slow CI worker doesn't flake; bounded so a stuck writer fails + // the test rather than hanging. Successful probes are released immediately so + // they don't add a lingering reader. + bool writer_pending = false; + for (int i = 0; i < 1000; ++i) { // up to ~10s + if (!m.try_lock_shared()) { + writer_pending = true; + break; + } + m.unlock_shared(); + bthread_usleep(10 * 1000); + } Review Comment: This polling loop can take up to ~10 seconds per run on worst-case scheduling, which can noticeably slow CI and amplify flakiness when machines are heavily loaded. Consider tightening the bound (e.g., fewer iterations / shorter sleep) and/or using a synchronization signal from the writer thread to reduce reliance on time-based polling (for example, adding a dedicated atomic/condvar handshake for when the writer has reached the pending state). ########## be/src/cloud/cloud_tablet.cpp: ########## @@ -188,7 +188,10 @@ Result<std::vector<Version>> CloudTablet::capture_versions_prefer_cache( const Version& spec_version) const { g_capture_prefer_cache_count << 1; Versions version_path; - std::shared_lock rlock(_meta_lock); + // Caller (capture_consistent_versions_unlocked) already holds shared + // `_meta_lock`; do NOT re-acquire it here. The lock is writer-preferring, + // so a recursive shared acquisition self-deadlocks if a writer queues in + // between the outer and inner lock. auto st = _timestamped_version_tracker.capture_consistent_versions_prefer_cache( spec_version, version_path, [&](int64_t start, int64_t end) { return rowset_is_warmed_up_unlocked(start, end); }); Review Comment: This method used to take a shared lock internally (per the removed `std::shared_lock rlock(_meta_lock);`) but now silently relies on the caller holding `_meta_lock`. Since the name/signature doesn’t indicate a lock-precondition, it’s easy to call this from a context that doesn’t already hold `_meta_lock`, which would be a correctness/concurrency hazard. Recommendation (mandatory): introduce a locking wrapper (e.g., keep this method locking, and add a `capture_versions_prefer_cache_unlocked()` variant), or rename this method to `*_unlocked` and update the header/docs so the lock contract is explicit. -- 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]
