Skip to content

Commit 32aea31

Browse files
enricogiorenricogior
committed
[Fix] Fork code for background processing fixes and code refactoring.
 - [Fix] BeginForkOperation_Aof()/_Rdb()/_Socket() and BeginForkOperation() code refactoring. - [Fix] rewriteAppendOnlyFileBackground() code refactoring to minimize the code changes for WIN32. - [Fix] rewriteAppendOnlyFileBackground() must update the latency monitor, the fork stats and replicationScriptCacheFlush(). - [Fix] rdbSaveBackground() code refactoring to minimize the code changes for WIN32. - [Fix] rdbSaveBackground() must update the latency monitor and the fork stats. - [Fix] memory leak in rdbSaveToSlavesSockets(). - [Fix] properly releasing resources in rdbSaveToSlavesSockets(). - [Fix] QForkChildInit() not setting the operationFailed event in case of exception. - [Fix] QForkChildInit() AV in catch() statement.
1 parent d82a361 commit 32aea31

File tree

6 files changed

+45
-91
lines changed

6 files changed

+45
-91
lines changed

‎src/Win32_Interop/Win32_QFork.cpp‎

Lines changed: 17 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -340,20 +340,17 @@ BOOL QForkChildInit(HANDLE QForkConrolMemoryMapHandle, DWORD ParentProcessID) {
340340
catch(std::system_error syserr) {
341341
if (ReportSpecialSystemErrors(syserr.code().value()) == false) {
342342
::redisLog(REDIS_WARNING, "QForkChildInit: system error caught. error code=0x%08x, message=%s\n", syserr.code().value(), syserr.what());
343-
g_pQForkControl = NULL;
344-
if (g_pQForkControl != NULL) {
345-
if (g_pQForkControl->operationFailed != NULL) {
346-
SetEvent(g_pQForkControl->operationFailed);
347-
}
348-
}
349-
return FALSE;
350343
}
351344
}
352345
catch(std::runtime_error runerr) {
353346
::redisLog(REDIS_WARNING, "QForkChildInit: runtime error caught. message=%s\n", runerr.what());
347+
}
348+
349+
if (g_pQForkControl != NULL) {
350+
if (g_pQForkControl->operationFailed != NULL) {
351+
SetEvent(g_pQForkControl->operationFailed);
352+
}
354353
g_pQForkControl = NULL;
355-
SetEvent(g_pQForkControl->operationFailed);
356-
return FALSE;
357354
}
358355
return FALSE;
359356
}
@@ -917,12 +914,13 @@ void CreateChildProcess(PROCESS_INFORMATION *pi, char* logfile, DWORD dwCreation
917914

918915
typedef void (*CHILD_PID_HOOK)(DWORD pid);
919916

920-
BOOL BeginForkOperation(OperationType type, LPVOID globalData, int sizeOfGlobalData, DWORD* childPID, uint32_t dictHashSeed, char* logfile, CHILD_PID_HOOK pidHook = NULL) {
917+
pid_t BeginForkOperation(OperationType type, LPVOID globalData, int sizeOfGlobalData, uint32_t dictHashSeed, char* logfile, CHILD_PID_HOOK pidHook = NULL) {
921918
PROCESS_INFORMATION pi;
922919
try {
923920
pi.hProcess = INVALID_HANDLE_VALUE;
921+
pi.dwProcessId = -1;
924922

925-
if(pidHook != NULL) {
923+
if (pidHook != NULL) {
926924
CreateChildProcess(&pi, logfile, CREATE_SUSPENDED);
927925
pidHook(pi.dwProcessId);
928926
CopyForkOperationData(type, globalData, sizeOfGlobalData, dictHashSeed);
@@ -932,7 +930,6 @@ BOOL BeginForkOperation(OperationType type, LPVOID globalData, int sizeOfGlobalD
932930
CreateChildProcess(&pi, logfile, 0);
933931
}
934932

935-
*childPID = pi.dwProcessId;
936933
CloseHandle(pi.hThread);
937934

938935
// wait for "forked" process to map memory
@@ -943,7 +940,7 @@ BOOL BeginForkOperation(OperationType type, LPVOID globalData, int sizeOfGlobalD
943940
"Forked Process did not respond in a timely manner.");
944941
}
945942

946-
return TRUE;
943+
return pi.dwProcessId;
947944
}
948945
catch(std::system_error syserr) {
949946
::redisLog(REDIS_WARNING, "BeginForkOperation: system error caught. error code=0x%08x, message=%s\n", syserr.code().value(), syserr.what());
@@ -954,34 +951,32 @@ BOOL BeginForkOperation(OperationType type, LPVOID globalData, int sizeOfGlobalD
954951
catch(...) {
955952
::redisLog(REDIS_WARNING, "BeginForkOperation: other exception caught.\n");
956953
}
957-
if(pi.hProcess != INVALID_HANDLE_VALUE) {
954+
if (pi.hProcess != INVALID_HANDLE_VALUE) {
958955
TerminateProcess(pi.hProcess, 1);
959956
}
960-
return FALSE;
957+
return -1;
961958
}
962959

963-
BOOL BeginForkOperation_Rdb(
960+
pid_t BeginForkOperation_Rdb(
964961
char *filename,
965962
LPVOID globalData,
966963
int sizeOfGlobalData,
967-
DWORD* childPID,
968964
unsigned __int32 dictHashSeed,
969965
char* logfile)
970966
{
971967
strcpy_s(g_pQForkControl->globalData.filename, filename);
972-
return BeginForkOperation(otRDB, globalData, sizeOfGlobalData, childPID, dictHashSeed, logfile);
968+
return BeginForkOperation(otRDB, globalData, sizeOfGlobalData, dictHashSeed, logfile);
973969
}
974970

975-
BOOL BeginForkOperation_Aof(
971+
pid_t BeginForkOperation_Aof(
976972
char *filename,
977973
LPVOID globalData,
978974
int sizeOfGlobalData,
979-
DWORD* childPID,
980975
unsigned __int32 dictHashSeed,
981976
char* logfile)
982977
{
983978
strcpy_s(g_pQForkControl->globalData.filename, filename);
984-
return BeginForkOperation(otAOF, globalData, sizeOfGlobalData, childPID, dictHashSeed, logfile);
979+
return BeginForkOperation(otAOF, globalData, sizeOfGlobalData, dictHashSeed, logfile);
985980
}
986981

987982
void BeginForkOperation_Socket_PidHook(DWORD dwProcessId) {
@@ -992,14 +987,13 @@ void BeginForkOperation_Socket_PidHook(DWORD dwProcessId) {
992987
}
993988
}
994989

995-
BOOL BeginForkOperation_Socket(
990+
pid_t BeginForkOperation_Socket(
996991
int *fds,
997992
int numfds,
998993
uint64_t *clientids,
999994
int pipe_write_fd,
1000995
LPVOID globalData,
1001996
int sizeOfGlobalData,
1002-
DWORD* childPID,
1003997
unsigned __int32 dictHashSeed,
1004998
char* logfile)
1005999
{
@@ -1015,7 +1009,6 @@ BOOL BeginForkOperation_Socket(
10151009
return BeginForkOperation(otSocket,
10161010
globalData,
10171011
sizeOfGlobalData,
1018-
childPID,
10191012
dictHashSeed,
10201013
logfile,
10211014
BeginForkOperation_Socket_PidHook);

‎src/Win32_Interop/Win32_QFork.h‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -66,30 +66,27 @@ StartupStatus QForkStartup(int argc, char** argv);
6666
BOOL QForkShutdown();
6767

6868
// For parent process use only
69-
BOOL BeginForkOperation_Rdb(
69+
pid_t BeginForkOperation_Rdb(
7070
char* fileName,
7171
LPVOID globalData,
7272
int sizeOfGlobalData,
73-
DWORD* childPID,
7473
unsigned __int32 dictHashSeed,
7574
char* logfile);
7675

77-
BOOL BeginForkOperation_Aof(
76+
pid_t BeginForkOperation_Aof(
7877
char* fileName,
7978
LPVOID globalData,
8079
int sizeOfGlobalData,
81-
DWORD* childPID,
8280
unsigned __int32 dictHashSeed,
8381
char* logfile);
8482

85-
BOOL BeginForkOperation_Socket(
83+
pid_t BeginForkOperation_Socket(
8684
int *fds,
8785
int numfds,
8886
uint64_t *clientids,
8987
int pipe_write_fd,
9088
LPVOID globalData,
9189
int sizeOfGlobalData,
92-
DWORD* childPID,
9390
unsigned __int32 dictHashSeed,
9491
char* logfile);
9592

‎src/Win32_Interop/win32_types.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,6 @@ typedef double PORT_LONGDOUBLE;
7272
#endif
7373

7474
/* The maximum possible size_t value has all bits set */
75-
#define MAX_SIZE_T (~(size_t)0)
76-
75+
#define MAX_SIZE_T (~(size_t)0)
7776

77+
typedef int pid_t;

‎src/Win32_Interop/win32fixes.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,6 @@ int getrusage(int who, struct rusage * rusage);
178178
#endif /*SIG_SETMASK*/
179179

180180
typedef void (*__p_sig_fn_t)(int);
181-
typedef DWORD pid_t;
182181

183182
#ifndef _SIGSET_T_
184183
#define _SIGSET_T_

‎src/aof.c‎

Lines changed: 12 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1100,49 +1100,27 @@ int rewriteAppendOnlyFile(char *filename) {
11001100
* finally will rename(2) the temp file in the actual file name.
11011101
* The the new file is reopened as the new append only file. Profit!
11021102
*/
1103-
#ifdef _WIN32
1104-
int rewriteAppendOnlyFileBackground(void) {
1105-
PORT_LONGLONG start;
1106-
char tmpfile[256];
1107-
1108-
if (server.aof_child_pid != -1) return REDIS_ERR;
1109-
start = ustime();
1110-
1111-
snprintf(tmpfile,256,"temp-rewriteaof-bg-%d.aof", (int) getpid());
1112-
if (BeginForkOperation_Aof(tmpfile, &server, sizeof(server), &server.aof_child_pid, dictGetHashFunctionSeed(), server.logfile) == FALSE) {
1113-
redisLog(REDIS_WARNING,
1114-
"Can't rewrite append only file in background: fork: %s",
1115-
strerror(errno));
1116-
return REDIS_ERR;
1117-
}
1118-
server.stat_fork_time = ustime()-start;
1119-
1120-
redisLog(REDIS_NOTICE,
1121-
"Background append only file rewriting started by pid %d",server.aof_child_pid);
1122-
server.aof_rewrite_scheduled = 0;
1123-
server.aof_rewrite_time_start = time(NULL);
1124-
updateDictResizePolicy();
1125-
/* We set appendseldb to -1 in order to force the next call to the
1126-
* feedAppendOnlyFile() to issue a SELECT command, so the differences
1127-
* accumulated by the parent into server.aof_rewrite_buf will start
1128-
* with a SELECT statement and it will be safe to merge. */
1129-
server.aof_selected_db = -1;
1130-
return REDIS_OK;
1131-
}
1132-
#else
11331103
int rewriteAppendOnlyFileBackground(void) {
11341104
pid_t childpid;
11351105
PORT_LONGLONG start;
11361106

11371107
if (server.aof_child_pid != -1) return REDIS_ERR;
11381108
start = ustime();
1109+
1110+
#ifndef _WIN32
11391111
if ((childpid = fork()) == 0) {
1112+
#endif
11401113
char tmpfile[256];
11411114

1142-
/* Child */
1115+
#ifndef _WIN32
1116+
/* Child */
11431117
closeListeningSockets(0);
11441118
redisSetProcTitle("redis-aof-rewrite");
1119+
#endif
11451120
snprintf(tmpfile,256,"temp-rewriteaof-bg-%d.aof", (int) getpid());
1121+
#ifdef _WIN32
1122+
childpid = BeginForkOperation_Aof(tmpfile, &server, sizeof(server), dictGetHashFunctionSeed(), server.logfile);
1123+
#else
11461124
if (rewriteAppendOnlyFile(tmpfile) == REDIS_OK) {
11471125
size_t private_dirty = zmalloc_get_private_dirty();
11481126

@@ -1156,6 +1134,7 @@ int rewriteAppendOnlyFileBackground(void) {
11561134
exitFromChild(1);
11571135
}
11581136
} else {
1137+
#endif
11591138
/* Parent */
11601139
server.stat_fork_time = ustime()-start;
11611140
server.stat_fork_rate = (double) zmalloc_used_memory() * 1000000 / server.stat_fork_time / (1024*1024*1024); /* GB per second. */
@@ -1179,10 +1158,11 @@ int rewriteAppendOnlyFileBackground(void) {
11791158
server.aof_selected_db = -1;
11801159
replicationScriptCacheFlush();
11811160
return REDIS_OK;
1161+
#ifndef _WIN32
11821162
}
1163+
#endif
11831164
return REDIS_OK; /* unreached */
11841165
}
1185-
#endif
11861166

11871167
void bgrewriteaofCommand(redisClient *c) {
11881168
if (server.aof_child_pid != -1) {

‎src/rdb.c‎

Lines changed: 11 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -787,24 +787,6 @@ int rdbSave(char *filename) {
787787
return REDIS_ERR;
788788
}
789789

790-
#ifdef _WIN32
791-
int rdbSaveBackground(char *filename) {
792-
PORT_LONGLONG start;
793-
start = ustime();
794-
server.dirty_before_bgsave = server.dirty;
795-
if (BeginForkOperation_Rdb(filename, &server, sizeof(server), &server.rdb_child_pid, dictGetHashFunctionSeed(), server.logfile)) {
796-
server.stat_fork_time = ustime()-start;
797-
server.rdb_child_type = REDIS_RDB_CHILD_TYPE_DISK;
798-
updateDictResizePolicy();
799-
return REDIS_OK;
800-
} else {
801-
redisLog(REDIS_WARNING,"Can't save in background: fork: %s", strerror(errno));
802-
return REDIS_ERR;
803-
}
804-
}
805-
806-
#else
807-
808790
int rdbSaveBackground(char *filename) {
809791
pid_t childpid;
810792
PORT_LONGLONG start;
@@ -815,6 +797,9 @@ int rdbSaveBackground(char *filename) {
815797
server.lastbgsave_try = time(NULL);
816798

817799
start = ustime();
800+
#ifdef _WIN32
801+
childpid = BeginForkOperation_Rdb(filename, &server, sizeof(server), dictGetHashFunctionSeed(), server.logfile);
802+
#else
818803
if ((childpid = fork()) == 0) {
819804
int retval;
820805

@@ -833,6 +818,7 @@ int rdbSaveBackground(char *filename) {
833818
}
834819
exitFromChild((retval == REDIS_OK) ? 0 : 1);
835820
} else {
821+
#endif
836822
/* Parent */
837823
server.stat_fork_time = ustime()-start;
838824
server.stat_fork_rate = (double) zmalloc_used_memory() * 1000000 / server.stat_fork_time / (1024*1024*1024); /* GB per second. */
@@ -849,10 +835,11 @@ int rdbSaveBackground(char *filename) {
849835
server.rdb_child_type = REDIS_RDB_CHILD_TYPE_DISK;
850836
updateDictResizePolicy();
851837
return REDIS_OK;
838+
#ifndef _WIN32
852839
}
840+
#endif
853841
return REDIS_OK; /* unreached */
854842
}
855-
#endif
856843

857844
void rdbRemoveTempFile(pid_t childpid) {
858845
char tmpfile[256];
@@ -1482,7 +1469,9 @@ int rdbSaveToSlavesSockets(void) {
14821469
/* Create the child process. */
14831470
start = ustime();
14841471

1485-
#ifndef _WIN32
1472+
#ifdef _WIN32
1473+
childpid = BeginForkOperation_Socket(fds, numfds, clientids, pipefds[1], &server, sizeof(server), dictGetHashFunctionSeed(), server.logfile);
1474+
#else
14861475
if ((childpid = fork()) == 0) {
14871476
/* Child */
14881477
int retval;
@@ -1549,12 +1538,6 @@ int rdbSaveToSlavesSockets(void) {
15491538
zfree(clientids);
15501539
exitFromChild((retval == REDIS_OK) ? 0 : 1);
15511540
} else {
1552-
#else // #ifndef _WIN32
1553-
if (!BeginForkOperation_Socket(fds, numfds, clientids, pipefds[1], &server, sizeof(server), &server.rdb_child_pid, dictGetHashFunctionSeed(), server.logfile)) {
1554-
redisLog(REDIS_WARNING,"Can't save in background: fork: %s", strerror(errno));
1555-
return REDIS_ERR;
1556-
} else {
1557-
childpid = server.rdb_child_pid;
15581541
#endif
15591542
/* Parent */
15601543
zfree(clientids); /* Not used by parent. Free ASAP. */
@@ -1576,7 +1559,9 @@ int rdbSaveToSlavesSockets(void) {
15761559
updateDictResizePolicy();
15771560
zfree(fds);
15781561
return REDIS_OK;
1562+
#ifndef _WIN32
15791563
}
1564+
#endif
15801565
return REDIS_OK; /* unreached */
15811566
}
15821567

0 commit comments

Comments
 (0)