Skip to content

Commit ceb9422

Browse files
trondnjimwwalker
authored andcommitted
MB-42657: Don't call shutdown_server from signal handler
shutdown_server isn't signal-safe. Instead we should just set the memcached_shutdown variable to true as that is signal safe. The clock callback happens every second and will poll the shutdown variable. Note that the sigterm and sigint handler isn't the "normal" way to initiate shutdown, that happens from: a) the parent process close stdin b) someone sends shutdown request Change-Id: I7cbbe0028643d6fdf747bdd710572d9279403bc3 Reviewed-on: http://review.couchbase.org/c/kv_engine/+/156471 Tested-by: Build Bot <[email protected]> Reviewed-by: Dave Rigby <[email protected]>
1 parent 33b6c12 commit ceb9422

File tree

4 files changed

+19
-35
lines changed

4 files changed

+19
-35
lines changed

daemon/mc_time.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -188,8 +188,8 @@ static void mc_time_clock_event_handler(evutil_socket_t fd, short which, void *a
188188
t.tv_usec = 0;
189189

190190
if (is_memcached_shutting_down()) {
191-
event_base_loopbreak(event_get_base(&clockevent));
192-
return ;
191+
stop_memcached_main_base();
192+
return;
193193
}
194194

195195
if (initialized) {

daemon/mcbp_executors.cc

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -518,9 +518,11 @@ static void get_errmap_executor(Cookie& cookie) {
518518

519519
static void shutdown_executor(Cookie& cookie) {
520520
if (session_cas.increment_session_counter(cookie.getRequest().getCas())) {
521-
shutdown_server();
522521
session_cas.decrement_session_counter();
523522
cookie.sendResponse(cb::mcbp::Status::Success);
523+
LOG_INFO("{} Shutdown server requested",
524+
cookie.getConnection().getId());
525+
shutdown_server();
524526
} else {
525527
cookie.sendResponse(cb::mcbp::Status::KeyEexists);
526528
}

daemon/memcached.cc

Lines changed: 13 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,10 @@ bool is_memcached_shutting_down() {
8080
return memcached_shutdown;
8181
}
8282

83+
void shutdown_server() {
84+
memcached_shutdown = true;
85+
}
86+
8387
/*
8488
* forward declarations
8589
*/
@@ -95,7 +99,13 @@ struct stats stats;
9599

96100
/** file scope variables **/
97101

98-
static std::unique_ptr<folly::EventBase> main_base;
102+
std::unique_ptr<folly::EventBase> main_base;
103+
104+
void stop_memcached_main_base() {
105+
if (main_base) {
106+
main_base->terminateLoopSoon();
107+
}
108+
}
99109

100110
static folly::Synchronized<std::string, std::mutex> reset_stats_time;
101111
/**
@@ -670,7 +680,7 @@ bool is_bucket_dying(Connection& c) {
670680
}
671681

672682
static void sigint_handler() {
673-
shutdown_server();
683+
memcached_shutdown = true;
674684
}
675685

676686
#ifdef WIN32
@@ -684,7 +694,7 @@ static void release_signal_handlers() {
684694
#else
685695

686696
static void sigterm_handler(evutil_socket_t, short, void *) {
687-
shutdown_server();
697+
memcached_shutdown = true;
688698
}
689699

690700
static struct event* sigterm_event;
@@ -715,28 +725,6 @@ const char* get_server_version() {
715725
return PRODUCT_VERSION;
716726
}
717727

718-
static std::condition_variable shutdown_cv;
719-
static std::mutex shutdown_cv_mutex;
720-
static bool memcached_can_shutdown = false;
721-
void shutdown_server() {
722-
723-
std::unique_lock<std::mutex> lk(shutdown_cv_mutex);
724-
if (!memcached_can_shutdown) {
725-
// log and proceed to wait shutdown
726-
LOG_INFO_RAW("shutdown_server waiting for can_shutdown signal");
727-
shutdown_cv.wait(lk, []{return memcached_can_shutdown;});
728-
}
729-
memcached_shutdown = true;
730-
LOG_INFO_RAW("Received shutdown request");
731-
main_base->terminateLoopSoon();
732-
}
733-
734-
void enable_shutdown() {
735-
std::unique_lock<std::mutex> lk(shutdown_cv_mutex);
736-
memcached_can_shutdown = true;
737-
shutdown_cv.notify_all();
738-
}
739-
740728
void cleanup_buckets() {
741729
for (auto &bucket : all_buckets) {
742730
bool waiting;
@@ -1032,13 +1020,6 @@ int memcached_main(int argc, char** argv) {
10321020
startStaleTraceDumpRemover(std::chrono::minutes(1),
10331021
std::chrono::minutes(5));
10341022

1035-
/*
1036-
* MB-20034.
1037-
* Now that all threads have been created, e.g. the audit thread, threads
1038-
* associated with extensions and the workers, we can enable shutdown.
1039-
*/
1040-
enable_shutdown();
1041-
10421023
/* Initialise memcached time keeping */
10431024
mc_time_init(main_base->getLibeventBase());
10441025

daemon/memcached.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ void scheduleDcpStep(Cookie& cookie);
5757
void safe_close(SOCKET sfd);
5858
const char* get_server_version();
5959
bool is_memcached_shutting_down();
60+
void stop_memcached_main_base();
6061

6162
/**
6263
* Connection-related functions

0 commit comments

Comments
 (0)