From 586a16ad7907d9742a63cfcec464be7ac54aa495 Mon Sep 17 00:00:00 2001
From: Binbin <binloveplay1314@qq.com>
Date: Fri, 13 May 2022 01:10:38 +0800
Subject: [PATCH] Fix race in module fork kill test (#10717)

The purpose of the test is to kill the child while it is running.
From the last two lines we can see the child exits before being killed.
```
- Module fork started pid: 56998
* <fork> fork child started
- Killing running module fork child: 56998
* <fork> fork child exiting
signal-handler (1652267501) Received SIGUSR1 in child, exiting now.
```

In this commit, we pass an argument to `fork.create` indicating how
long it should sleep. For the fork kill test, we use a longer time to
avoid the child exiting before being killed.

Other changes:
use wait_for_condition instead of hardcoded `after 250`.
Unify the test for failing fork with the one for killing it (save time)
---
 tests/modules/fork.c          | 13 ++++++++-----
 tests/unit/moduleapi/fork.tcl | 26 +++++++++++++++-----------
 2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/tests/modules/fork.c b/tests/modules/fork.c
index 44e3971bd..d7a0d154f 100644
--- a/tests/modules/fork.c
+++ b/tests/modules/fork.c
@@ -23,7 +23,8 @@ void done_handler(int exitcode, int bysignal, void *user_data) {
 int fork_create(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
 {
     long long code_to_exit_with;
-    if (argc != 2) {
+    long long usleep_us;
+    if (argc != 3) {
         RedisModule_WrongArity(ctx);
         return REDISMODULE_OK;
     }
@@ -34,20 +35,22 @@ int fork_create(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
     }
 
     RedisModule_StringToLongLong(argv[1], &code_to_exit_with);
+    RedisModule_StringToLongLong(argv[2], &usleep_us);
     exitted_with_code = -1;
-    child_pid = RedisModule_Fork(done_handler, (void*)0xdeadbeef);
-    if (child_pid < 0) {
+    int fork_child_pid = RedisModule_Fork(done_handler, (void*)0xdeadbeef);
+    if (fork_child_pid < 0) {
         RedisModule_ReplyWithError(ctx, "Fork failed");
         return REDISMODULE_OK;
-    } else if (child_pid > 0) {
+    } else if (fork_child_pid > 0) {
         /* parent */
+        child_pid = fork_child_pid;
         RedisModule_ReplyWithLongLong(ctx, child_pid);
         return REDISMODULE_OK;
     }
 
     /* child */
     RedisModule_Log(ctx, "notice", "fork child started");
-    usleep(500000);
+    usleep(usleep_us);
     RedisModule_Log(ctx, "notice", "fork child exiting");
     RedisModule_ExitFromChild(code_to_exit_with);
     /* unreachable */
diff --git a/tests/unit/moduleapi/fork.tcl b/tests/unit/moduleapi/fork.tcl
index d6a2db9a9..c89a6c524 100644
--- a/tests/unit/moduleapi/fork.tcl
+++ b/tests/unit/moduleapi/fork.tcl
@@ -13,7 +13,8 @@ start_server {tags {"modules"}} {
 
     test {Module fork} {
         # the argument to fork.create is the exitcode on termination
-        r fork.create 3
+        # the second argument to fork.create is passed to usleep
+        r fork.create 3 100000 ;# 100ms
         wait_for_condition 20 100 {
             [r fork.exitcode] != -1
         } else {
@@ -23,22 +24,25 @@ start_server {tags {"modules"}} {
     } {3}
 
     test {Module fork kill} {
-        r fork.create 3
-        after 250
+        # use a longer time to avoid the child exiting before being killed
+        r fork.create 3 100000000 ;# 100s
+        wait_for_condition 20 100 {
+            [count_log_message "fork child started"] == 2
+        } else {
+            fail "fork didn't start"
+        }
+
+        # module fork twice
+        assert_error {Fork failed} {r fork.create 0 1}
+        assert {[count_log_message "Can't fork for module: File exists"] eq "1"}
+
         r fork.kill
 
-        assert {[count_log_message "fork child started"] eq "2"}
         assert {[count_log_message "Received SIGUSR1 in child"] eq "1"}
+        # check that it wasn't printed again (the print belong to the previous test)
         assert {[count_log_message "fork child exiting"] eq "1"}
     }
 
-    test {Module fork twice} {
-        r fork.create 0
-        after 250
-        catch {r fork.create 0}
-        assert {[count_log_message "Can't fork for module: File exists"] eq "1"}
-    }
-
     test "Unload the module - fork" {
         assert_equal {OK} [r module unload fork]
     }