Skip to content

Commit

Permalink
Fix daylight race condition and some thread leaks (redis#13191)
Browse files Browse the repository at this point in the history
fix some issues that come from sanitizer thread report.

1. when the main thread is updating daylight_active, other threads (bio,
module thread) may be writing logs at the same time.
```
WARNING: ThreadSanitizer: data race (pid=661064)
  Read of size 4 at 0x55c9a4d11c70 by thread T2:
    #0 serverLogRaw /home/sundb/data/redis_fork/src/server.c:116 (redis-server+0x8d797) (BuildId: dca0b1945ba30010e36129bdb296e488dd2b32d0)
    #1 _serverLog.constprop.2 /home/sundb/data/redis_fork/src/server.c:146 (redis-server+0x2a3b14) (BuildId: dca0b1945ba30010e36129bdb296e488dd2b32d0)
    #2 bioProcessBackgroundJobs /home/sundb/data/redis_fork/src/bio.c:329 (redis-server+0x1c24ca) (BuildId: dca0b1945ba30010e36129bdb296e488dd2b32d0)

  Previous write of size 4 at 0x55c9a4d11c70 by main thread (mutexes: write M0, write M1, write M2, write M3):
    #0 updateCachedTimeWithUs /home/sundb/data/redis_fork/src/server.c:1102 (redis-server+0x925e7) (BuildId: dca0b1945ba30010e36129bdb296e488dd2b32d0)
    #1 updateCachedTimeWithUs /home/sundb/data/redis_fork/src/server.c:1087 (redis-server+0x925e7)
    #2 updateCachedTime /home/sundb/data/redis_fork/src/server.c:1118 (redis-server+0x925e7)
    #3 afterSleep /home/sundb/data/redis_fork/src/server.c:1811 (redis-server+0x925e7)
    #4 aeProcessEvents /home/sundb/data/redis_fork/src/ae.c:389 (redis-server+0x85ae0) (BuildId: dca0b1945ba30010e36129bdb296e488dd2b32d0)
    #5 aeProcessEvents /home/sundb/data/redis_fork/src/ae.c:342 (redis-server+0x85ae0)
    #6 aeMain /home/sundb/data/redis_fork/src/ae.c:477 (redis-server+0x85ae0)
    #7 main /home/sundb/data/redis_fork/src/server.c:7211 (redis-server+0x7168c) (BuildId: dca0b1945ba30010e36129bdb296e488dd2b32d0)
```

2. thread leaks in module tests
```
WARNING: ThreadSanitizer: thread leak (pid=668683)
  Thread T13 (tid=670041, finished) created by main thread at:
    #0 pthread_create ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:1036 (libtsan.so.2+0x3d179) (BuildId: 28a9f70061dbb2dfa2cef661d3b23aff4ea13536)
    #1 HelloBlockNoTracking_RedisCommand /home/sundb/data/redis_fork/tests/modules/blockonbackground.c:200 (blockonbackground.so+0x97fd) (BuildId: 9cd187906c57e88cdf896d121d1d96448b37a136)
    #2 HelloBlockNoTracking_RedisCommand /home/sundb/data/redis_fork/tests/modules/blockonbackground.c:169 (blockonbackground.so+0x97fd)
    #3 call /home/sundb/data/redis_fork/src/server.c:3546 (redis-server+0x9b7fb) (BuildId: dca0b1945ba30010e36129bdb296e488dd2b32d0)
    #4 processCommand /home/sundb/data/redis_fork/src/server.c:4176 (redis-server+0xa091c) (BuildId: dca0b1945ba30010e36129bdb296e488dd2b32d0)
    #5 processCommandAndResetClient /home/sundb/data/redis_fork/src/networking.c:2468 (redis-server+0xd2b8e) (BuildId: dca0b1945ba30010e36129bdb296e488dd2b32d0)
    #6 processInputBuffer /home/sundb/data/redis_fork/src/networking.c:2576 (redis-server+0xd2b8e)
    #7 readQueryFromClient /home/sundb/data/redis_fork/src/networking.c:2722 (redis-server+0xd358f) (BuildId: dca0b1945ba30010e36129bdb296e488dd2b32d0)
    #8 callHandler /home/sundb/data/redis_fork/src/connhelpers.h:58 (redis-server+0x288a7b) (BuildId: dca0b1945ba30010e36129bdb296e488dd2b32d0)
    #9 connSocketEventHandler /home/sundb/data/redis_fork/src/socket.c:277 (redis-server+0x288a7b)
    #10 aeProcessEvents /home/sundb/data/redis_fork/src/ae.c:417 (redis-server+0x85b45) (BuildId: dca0b1945ba30010e36129bdb296e488dd2b32d0)
    #11 aeProcessEvents /home/sundb/data/redis_fork/src/ae.c:342 (redis-server+0x85b45)
    #12 aeMain /home/sundb/data/redis_fork/src/ae.c:477 (redis-server+0x85b45)
    #13 main /home/sundb/data/redis_fork/src/server.c:7211 (redis-server+0x7168c) (BuildId: dca0b1945ba30010e36129bdb296e488dd2b32d0)
```
  • Loading branch information
sundb committed Apr 4, 2024
1 parent 4df0379 commit 4581d43
Show file tree
Hide file tree
Showing 8 changed files with 16 additions and 5 deletions.
6 changes: 4 additions & 2 deletions src/server.c
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,13 @@ void serverLogRaw(int level, const char *msg) {
int off;
struct timeval tv;
int role_char;
int daylight_active = 0;
pid_t pid = getpid();

gettimeofday(&tv,NULL);
struct tm tm;
nolocks_localtime(&tm,tv.tv_sec,server.timezone,server.daylight_active);
atomicGet(server.daylight_active, daylight_active);
nolocks_localtime(&tm,tv.tv_sec,server.timezone,daylight_active);
off = strftime(buf,sizeof(buf),"%d %b %Y %H:%M:%S.",&tm);
snprintf(buf+off,sizeof(buf)-off,"%03d",(int)tv.tv_usec/1000);
if (server.sentinel_mode) {
Expand Down Expand Up @@ -1099,7 +1101,7 @@ static inline void updateCachedTimeWithUs(int update_daylight_info, const long l
struct tm tm;
time_t ut = server.unixtime;
localtime_r(&ut,&tm);
server.daylight_active = tm.tm_isdst;
atomicSet(server.daylight_active, tm.tm_isdst);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -1962,7 +1962,7 @@ struct redisServer {
/* time cache */
redisAtomic time_t unixtime; /* Unix time sampled every cron cycle. */
time_t timezone; /* Cached timezone. As set by tzset(). */
int daylight_active; /* Currently in daylight saving time. */
redisAtomic int daylight_active; /* Currently in daylight saving time. */
mstime_t mstime; /* 'unixtime' in milliseconds. */
ustime_t ustime; /* 'unixtime' in microseconds. */
mstime_t cmd_time_snapshot; /* Time snapshot of the root execution nesting. */
Expand Down
1 change: 1 addition & 0 deletions tests/modules/auth.c
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ int blocking_auth_cb(RedisModuleCtx *ctx, RedisModuleString *username, RedisModu
if (pthread_create(&tid, NULL, AuthBlock_ThreadMain, targ) != 0) {
RedisModule_AbortBlock(bc);
}
pthread_detach(tid);
return REDISMODULE_AUTH_HANDLED;
}

Expand Down
3 changes: 3 additions & 0 deletions tests/modules/blockedclient.c
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ int acquire_gil(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
pthread_t tid;
int res = pthread_create(&tid, NULL, worker, bc);
assert(res == 0);
pthread_detach(tid);

return REDISMODULE_OK;
}
Expand Down Expand Up @@ -195,6 +196,7 @@ int do_bg_rm_call(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
pthread_t tid;
int res = pthread_create(&tid, NULL, bg_call_worker, bg);
assert(res == 0);
pthread_detach(tid);

return REDISMODULE_OK;
}
Expand Down Expand Up @@ -344,6 +346,7 @@ static void rm_call_async_reply_on_thread(RedisModuleCtx *ctx, RedisModuleCallRe
pthread_t tid;
int res = pthread_create(&tid, NULL, send_async_reply, ta_rm_call_ctx);
assert(res == 0);
pthread_detach(tid);
}

/*
Expand Down
3 changes: 3 additions & 0 deletions tests/modules/blockonbackground.c
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ int HelloBlock_RedisCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int a
RedisModule_AbortBlock(bc);
return RedisModule_ReplyWithError(ctx,"-ERR Can't start thread");
}
pthread_detach(tid);
return REDISMODULE_OK;
}

Expand Down Expand Up @@ -201,6 +202,7 @@ int HelloBlockNoTracking_RedisCommand(RedisModuleCtx *ctx, RedisModuleString **a
RedisModule_AbortBlock(bc);
return RedisModule_ReplyWithError(ctx,"-ERR Can't start thread");
}
pthread_detach(tid);
return REDISMODULE_OK;
}

Expand Down Expand Up @@ -231,6 +233,7 @@ int HelloDoubleBlock_RedisCommand(RedisModuleCtx *ctx, RedisModuleString **argv,
RedisModule_AbortBlock(bc);
return RedisModule_ReplyWithError(ctx,"-ERR Can't start thread");
}
pthread_detach(tid);
return REDISMODULE_OK;
}

Expand Down
1 change: 1 addition & 0 deletions tests/modules/postnotifications.c
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ static int KeySpace_PostNotificationsAsyncSet(RedisModuleCtx *ctx, RedisModuleSt
RedisModule_AbortBlock(bc);
return RedisModule_ReplyWithError(ctx,"-ERR Can't start thread");
}
pthread_detach(tid);
return REDISMODULE_OK;
}

Expand Down
4 changes: 2 additions & 2 deletions tests/modules/propagate.c
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ int propagateTestThreadCommand(RedisModuleCtx *ctx, RedisModuleString **argv, in
pthread_t tid;
if (pthread_create(&tid,NULL,threadMain,NULL) != 0)
return RedisModule_ReplyWithError(ctx,"-ERR Can't start thread");
REDISMODULE_NOT_USED(tid);
pthread_detach(tid);

RedisModule_ReplyWithSimpleString(ctx,"OK");
return REDISMODULE_OK;
Expand Down Expand Up @@ -216,7 +216,7 @@ int propagateTestDetachedThreadCommand(RedisModuleCtx *ctx, RedisModuleString **
pthread_t tid;
if (pthread_create(&tid,NULL,threadDetachedMain,NULL) != 0)
return RedisModule_ReplyWithError(ctx,"-ERR Can't start thread");
REDISMODULE_NOT_USED(tid);
pthread_detach(tid);

RedisModule_ReplyWithSimpleString(ctx,"OK");
return REDISMODULE_OK;
Expand Down
1 change: 1 addition & 0 deletions tests/modules/usercall.c
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ int call_with_user_bg(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
pthread_t tid;
int res = pthread_create(&tid, NULL, bg_call_worker, bg);
assert(res == 0);
pthread_detach(tid);

return REDISMODULE_OK;
}
Expand Down

0 comments on commit 4581d43

Please sign in to comment.