Skip to content

Commit fb6944d

Browse files
committed
MB-36051: Make Connection::priority atomic
As reported by TSan, Connection::priority is read and written without a lock: hreadSanitizer: data race third_party/gsl-lite/include/gsl/gsl-lite.h:472gsl::fail_fast_assert(bool, char const*) Read of size 1 at 0x7b5c00019738 by thread T23 (mutexes: write M2903806): #0 gsl::fail_fast_assert(bool, char const*) third_party/gsl-lite/include/gsl/gsl-lite.h:472 (memcached+0x000000465448) couchbase#1 gsl::not_null::get() const third_party/gsl-lite/include/gsl/gsl-lite.h:888 (memcached+0x000000465448) couchbase#2 ServerCookieApi::get_priority(gsl::not_null) kv_engine/daemon/memcached.cc:1640 (memcached+0x000000465448) couchbase#3 EventuallyPersistentEngine::getDCPPriority(void const*) kv_engine/engines/ep/src/ep_engine.cc:5828 (ep.so+0x000000171c57) couchbase#4 ConnHandler::addStats(std::function)> const&, void const*) kv_engine/engines/ep/src/connhandler.cc:327 (ep.so+0x000000092b33) couchbase#5 DcpProducer::addStats(std::function)> const&, void const*) kv_engine/engines/ep/src/dcp/producer.cc:1252 (ep.so+0x000000119b1e) couchbase#6 ConnStatBuilder::operator()(std::shared_ptr) kv_engine/engines/ep/src/ep_engine.cc:3571 (ep.so+0x000000191fbf) #7 void DcpConnMap::each(ConnStatBuilder) kv_engine/engines/ep/src/dcp/dcpconnmap.h:164 (ep.so+0x000000191fbf) #8 EventuallyPersistentEngine::doDcpStats(void const*, std::function)> const&, cb::const_char_buffer) kv_engine/engines/ep/src/ep_engine.cc:3728 (ep.so+0x000000191fbf) #9 EventuallyPersistentEngine::getStats(void const*, cb::const_char_buffer, cb::const_char_buffer, std::function)> const&) kv_engine/engines/ep/src/ep_engine.cc:4398 (ep.so+0x000000192bb8) #10 KVBucket::snapshotStats() kv_engine/engines/ep/src/kv_bucket.cc:1149 (ep.so+0x0000002116db) #11 StatSnap::run() kv_engine/engines/ep/src/tasks.cc:72 (ep.so+0x000000258ae2) #12 ExecutorThread::run() kv_engine/engines/ep/src/executorthread.cc:153 (ep.so+0x0000001d0405) ... Previous write of size 1 at 0x7b5c00019738 by thread T7 (mutexes: write M1036245144598543240): #0 Connection::setPriority(Connection::Priority) kv_engine/daemon/connection.cc:1593 (memcached+0x0000004be2f2) couchbase#1 ServerCookieApi::set_priority(gsl::not_null, CONN_PRIORITY) kv_engine/daemon/memcached.cc:1618 (memcached+0x0000004658b9) couchbase#2 EventuallyPersistentEngine::setDCPPriority(void const*, CONN_PRIORITY) kv_engine/engines/ep/src/ep_engine.cc:5835 (ep.so+0x000000171d5f) couchbase#3 DcpProducer::control(unsigned int, cb::const_char_buffer, cb::const_char_buffer) kv_engine/engines/ep/src/dcp/producer.cc:945 (ep.so+0x000000118cc3) couchbase#4 non-virtual thunk to EventuallyPersistentEngine::control(gsl::not_null, unsigned int, cb::const_char_buffer, cb::const_char_buffer) (ep.so+0x00000018f966) couchbase#5 dcpControl(Cookie&, unsigned int, cb::const_char_buffer, cb::const_char_buffer) kv_engine/daemon/protocol/mcbp/engine_wrapper.cc:408 (memcached+0x00000047c981) couchbase#6 dcp_control_executor(Cookie&) kv_engine/daemon/protocol/mcbp/dcp_control_executor.cc:38 (memcached+0x000000513421) #7 std::_Function_handler::_M_invoke(std::_Any_data const&, Cookie&) /usr/local/include/c++/7.3.0/bits/std_function.h:316 (memcached+0x000000503202) #8 std::function::operator()(Cookie&) const /usr/local/include/c++/7.3.0/bits/std_function.h:706 (memcached+0x0000005019fe) #9 execute_client_request_packet(Cookie&, cb::mcbp::Request const&) kv_engine/daemon/mcbp_executors.cc:857 (memcached+0x0000005019fe) #10 execute_request_packet(Cookie&, cb::mcbp::Request const&) kv_engine/daemon/mcbp_executors.cc:881 (memcached+0x000000501bd7) ... Fix by changing Connection::priority to be an atomic. Change-Id: Ia6bae64ec09e1963e55e7cdb614fb17a68ff1726 Reviewed-on: http://review.couchbase.org/114956 Reviewed-by: Trond Norbye <trond.norbye@couchbase.com> Tested-by: Build Bot <build@couchbase.com>
1 parent ec02585 commit fb6944d

File tree

2 files changed

+8
-4
lines changed

2 files changed

+8
-4
lines changed

daemon/connection.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -1590,7 +1590,7 @@ bool Connection::signalIfIdle() {
15901590
}
15911591

15921592
void Connection::setPriority(Connection::Priority priority) {
1593-
Connection::priority = priority;
1593+
Connection::priority.store(priority);
15941594
switch (priority) {
15951595
case Priority::High:
15961596
max_reqs_per_event =

daemon/connection.h

+7-3
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ class Connection : public dcp_message_producers {
194194
void setAuthenticated(bool authenticated);
195195

196196
Priority getPriority() const {
197-
return priority;
197+
return priority.load();
198198
}
199199

200200
void setPriority(const Priority priority);
@@ -1115,8 +1115,12 @@ class Connection : public dcp_message_producers {
11151115
/** Name of the local socket if known */
11161116
const std::string sockname;
11171117

1118-
/** The connections priority */
1119-
Priority priority{Priority::Medium};
1118+
/**
1119+
* The connections' priority.
1120+
* atomic to allow read (from DCP stats) without acquiring any
1121+
* additional locks (priority should rarely change).
1122+
*/
1123+
std::atomic<Priority> priority{Priority::Medium};
11201124

11211125
/** The cluster map revision used by this client */
11221126
int clustermap_revno{-2};

0 commit comments

Comments
 (0)