Skip to content

Commit

Permalink
MB-36051: Make Connection::priority atomic
Browse files Browse the repository at this point in the history
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)
    #1 gsl::not_null::get() const third_party/gsl-lite/include/gsl/gsl-lite.h:888 (memcached+0x000000465448)
    #2 ServerCookieApi::get_priority(gsl::not_null) kv_engine/daemon/memcached.cc:1640 (memcached+0x000000465448)
    #3 EventuallyPersistentEngine::getDCPPriority(void const*) kv_engine/engines/ep/src/ep_engine.cc:5828 (ep.so+0x000000171c57)
    #4 ConnHandler::addStats(std::function)> const&, void const*) kv_engine/engines/ep/src/connhandler.cc:327 (ep.so+0x000000092b33)
    #5 DcpProducer::addStats(std::function)> const&, void const*) kv_engine/engines/ep/src/dcp/producer.cc:1252 (ep.so+0x000000119b1e)
    #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)
    #1 ServerCookieApi::set_priority(gsl::not_null, CONN_PRIORITY) kv_engine/daemon/memcached.cc:1618 (memcached+0x0000004658b9)
    #2 EventuallyPersistentEngine::setDCPPriority(void const*, CONN_PRIORITY) kv_engine/engines/ep/src/ep_engine.cc:5835 (ep.so+0x000000171d5f)
    #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)
    #4 non-virtual thunk to EventuallyPersistentEngine::control(gsl::not_null, unsigned int, cb::const_char_buffer, cb::const_char_buffer)  (ep.so+0x00000018f966)
    #5 dcpControl(Cookie&, unsigned int, cb::const_char_buffer, cb::const_char_buffer) kv_engine/daemon/protocol/mcbp/engine_wrapper.cc:408 (memcached+0x00000047c981)
    #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 <[email protected]>
Tested-by: Build Bot <[email protected]>
  • Loading branch information
daverigby committed Sep 18, 2019
1 parent ec02585 commit fb6944d
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 4 deletions.
2 changes: 1 addition & 1 deletion daemon/connection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1590,7 +1590,7 @@ bool Connection::signalIfIdle() {
}

void Connection::setPriority(Connection::Priority priority) {
Connection::priority = priority;
Connection::priority.store(priority);
switch (priority) {
case Priority::High:
max_reqs_per_event =
Expand Down
10 changes: 7 additions & 3 deletions daemon/connection.h
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ class Connection : public dcp_message_producers {
void setAuthenticated(bool authenticated);

Priority getPriority() const {
return priority;
return priority.load();
}

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

/** The connections priority */
Priority priority{Priority::Medium};
/**
* The connections' priority.
* atomic to allow read (from DCP stats) without acquiring any
* additional locks (priority should rarely change).
*/
std::atomic<Priority> priority{Priority::Medium};

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

0 comments on commit fb6944d

Please sign in to comment.