Skip to content

Commit ac93972

Browse files
committed
fix race crash for accepting new connections
Inspired by a patch by Kazuki Ohta. Summary from a mail sent to the list by Shigeki: [Example Scenario] 1. throw alot clients (well over connection limit) to connect to memcached. 2. memcached's file descriptors reaches maximum setting 3. main thread calls accept_new_conns(false) to stop polling sfd 4. main thread's event_base_loop stops accepting incoming request 5. main thread stops to acceess main_base at this point 6. a client disconnects 7. worker thread calls accept_new_conns(true) to start polling sfd 8. accept_new_conns uses mutex to protect main_base's race condition 9. worker thread starts loop with listen_conn 10. worker thread calls update_event with first conn 11. after first update_event(), main thread start polling sfd and starts to access main_base <- PROBLEM 12. Worker thread continues to call update_event() with second conn At this point, worker thread and main thread both acccess and modify main_base. --- The original patch coupled polling with the once-per-second clock timer. My patch creates a 10ms poller which kicks off after the listener is disabled. Switching for a conditional would be too much rewiring for 1.4, as 1.6 solves this in a better way.
1 parent 0d45e55 commit ac93972

File tree

1 file changed

+26
-1
lines changed

1 file changed

+26
-1
lines changed

memcached.c

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,27 @@ enum transmit_result {
120120

121121
static enum transmit_result transmit(conn *c);
122122

123+
/* This reduces the latency without adding lots of extra wiring to be able to
124+
* notify the listener thread of when to listen again.
125+
* Also, the clock timer could be broken out into its own thread and we
126+
* can block the listener via a condition.
127+
*/
128+
static volatile bool allow_new_conns = true;
129+
static struct event maxconnsevent;
130+
static void maxconns_handler(const int fd, const short which, void *arg) {
131+
struct timeval t = {.tv_sec = 0, .tv_usec = 10000};
132+
133+
if (allow_new_conns == false) {
134+
/* reschedule in 10ms if we need to keep polling */
135+
evtimer_set(&maxconnsevent, maxconns_handler, 0);
136+
event_base_set(main_base, &maxconnsevent);
137+
evtimer_add(&maxconnsevent, &t);
138+
} else {
139+
evtimer_del(&maxconnsevent);
140+
accept_new_conns(true);
141+
}
142+
}
143+
123144
#define REALTIME_MAXDELTA 60*60*24*30
124145

125146
/*
@@ -509,7 +530,9 @@ static void conn_close(conn *c) {
509530

510531
MEMCACHED_CONN_RELEASE(c->sfd);
511532
close(c->sfd);
512-
accept_new_conns(true);
533+
pthread_mutex_lock(&conn_lock);
534+
allow_new_conns = true;
535+
pthread_mutex_unlock(&conn_lock);
513536
conn_cleanup(c);
514537

515538
/* if the connection has big buffers, just free it */
@@ -3363,6 +3386,8 @@ void do_accept_new_conns(const bool do_accept) {
33633386
stats.accepting_conns = false;
33643387
stats.listen_disabled_num++;
33653388
STATS_UNLOCK();
3389+
allow_new_conns = false;
3390+
maxconns_handler(0, 0, 0);
33663391
}
33673392
}
33683393

0 commit comments

Comments
 (0)