Skip to content

Commit

Permalink
Fix race accessing settings.verbosity
Browse files Browse the repository at this point in the history
As reported by ThreadSanitizer:

WARNING: ThreadSanitizer: data race (pid=34992)
  Read of size 4 at 0x7f98b38b5bf4 by main thread:
    #0 run_event_loop /home/daver/repos/couchbase/server/memcached/daemon/connections.cc:170 (memcached+0x0000000c7049)
    #1 event_handler /home/daver/repos/couchbase/server/memcached/daemon/memcached.cc:6912 (memcached+0x0000000d7b34)
    #2 event_persist_closure /home/couchbase/jenkins/workspace/cbdeps-build/label/ubuntu14.04/release/sherlock/deps/packages/build/libevent/libevent-prefix/src/libevent/event.c:1319 (libevent_core-2.0.so.5+0x00000000b6c7)
    #3 __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287 (libc.so.6+0x000000021ec4)

  Previous write of size 4 at 0x7f98b38b5bf4 by thread T19 (mutexes: write M45860):
    #0 verbosity_executor(conn*, void*) /home/daver/repos/couchbase/server/memcached/daemon/memcached.cc:3600 (memcached+0x0000000df219)
    #1 process_bin_packet(conn*) /home/daver/repos/couchbase/server/memcached/daemon/memcached.cc:5053 (memcached+0x0000000d2100)
    #2 run_event_loop /home/daver/repos/couchbase/server/memcached/daemon/connections.cc:174 (memcached+0x0000000c70c3)
    #3 event_handler /home/daver/repos/couchbase/server/memcached/daemon/memcached.cc:6912 (memcached+0x0000000d7b34)
    #4 event_persist_closure /home/couchbase/jenkins/workspace/cbdeps-build/label/ubuntu14.04/release/sherlock/deps/packages/build/libevent/libevent-prefix/src/libevent/event.c:1319 (libevent_core-2.0.so.5+0x00000000b6c7)
    #5 platform_thread_wrap /home/daver/repos/couchbase/server/platform/src/cb_pthreads.c:23 (libplatform.so.0.1.0+0x000000003730)

Change-Id: I2ae83312a532c67b2e3915e0c604e4e60558a8a7
Reviewed-on: http://review.couchbase.org/52487
Tested-by: buildbot <[email protected]>
Reviewed-by: Trond Norbye <[email protected]>
  • Loading branch information
daverigby authored and trondn committed Jul 1, 2015
1 parent c3a3d93 commit cd1f25e
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 17 deletions.
5 changes: 1 addition & 4 deletions daemon/breakpad.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,13 @@
# else
# error Unsupported platform for breakpad, cannot compile.
# endif
#include "settings.h"

#include "memcached/extension_loggers.h"
#include <platform/backtrace.h>

#include <stdlib.h>

extern "C" {
extern struct settings settings;
}

using namespace google_breakpad;

ExceptionHandler* handler;
Expand Down
13 changes: 10 additions & 3 deletions daemon/config_parse.cc
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ static bool get_absolute_file(const char *file, const char **value,
}


/** Given a JSON element {i} with the name {key}, attempt to convert
* it's value to an integer and store the result in {value}. Returns
* true on success; else returns false and sets {error_msg} to point
* to message describing the any error.
*/
static bool get_int_value(cJSON *i, const char *key, int* value,
char **error_msg) {
switch (i->type) {
Expand Down Expand Up @@ -370,7 +375,9 @@ static bool get_max_packet_size(cJSON *o, struct settings *settings,

static bool get_verbosity(cJSON *o, struct settings *settings,
char **error_msg) {
if (get_int_value(o, o->string, &settings->verbose, error_msg)) {
int verbosity;
if (get_int_value(o, o->string, &verbosity, error_msg)) {
settings->verbose.store(verbosity);
settings->has.verbose = true;
return true;
} else {
Expand Down Expand Up @@ -1331,11 +1338,11 @@ static void dyna_reconfig_verbosity(const struct settings *new_settings) {
if (new_settings->has.verbose &&
new_settings->verbose != settings.verbose) {
int old_verbose = settings.verbose;
settings.verbose = new_settings->verbose;
settings.verbose.store(new_settings->verbose);
perform_callbacks(ON_LOG_LEVEL, NULL, NULL);
settings.extensions.logger->log(EXTENSION_LOG_NOTICE, NULL,
"Changed verbosity from %d to %d", old_verbose,
settings.verbose);
settings.verbose.load());
}
}

Expand Down
4 changes: 2 additions & 2 deletions daemon/memcached.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5657,7 +5657,7 @@ static void process_stat_settings(ADD_STAT add_stats, void *c) {
}
}

APPEND_STAT("verbosity", "%d", settings.verbose);
APPEND_STAT("verbosity", "%d", settings.verbose.load());
APPEND_STAT("num_threads", "%d", settings.num_threads);
APPEND_STAT("reqs_per_event_high_priority", "%d",
settings.reqs_per_event_high_priority);
Expand Down Expand Up @@ -7713,7 +7713,7 @@ static EXTENSION_LOGGER_DESCRIPTOR* get_logger(void)
static EXTENSION_LOG_LEVEL get_log_level(void)
{
EXTENSION_LOG_LEVEL ret;
switch (settings.verbose) {
switch (settings.verbose.load()) {
case 0: ret = EXTENSION_LOG_NOTICE; break;
case 1: ret = EXTENSION_LOG_INFO; break;
case 2: ret = EXTENSION_LOG_DEBUG; break;
Expand Down
10 changes: 2 additions & 8 deletions daemon/settings.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@

#include <memcached/engine.h>

#ifdef __cplusplus
extern "C" {
#endif
#include <atomic>

/**
* An enumeration with constants for the various protocols supported
Expand Down Expand Up @@ -94,7 +92,7 @@ struct settings {
const char *rbac_file; /* The file containing RBAC information */
bool rbac_privilege_debug; /* see manpage */
bool require_sasl; /* require SASL auth */
int verbose; /* level of versosity to log at. */
std::atomic<int> verbose; /* level of versosity to log at. */
int bio_drain_buffer_sz; /* size of the SSL bio buffers */
bool datatype; /* is datatype support enabled? */
const char *root; /* The root directory of the installation */
Expand Down Expand Up @@ -169,7 +167,3 @@ struct settings {
};

extern struct settings settings;

#ifdef __cplusplus
} // extern "C"
#endif

0 comments on commit cd1f25e

Please sign in to comment.