Optimize the return value of the System::waitSignal() function to r…

…eturn the numerical value of the triggered signal instead of `true`.
swoole · Dec 4, 2024 · 367da9d · 367da9d
1 parent c086ddf
commit 367da9d
Show file tree

Hide file tree

Showing 8 changed files with 40 additions and 25 deletions.
diff --git a/ext-src/stubs/php_swoole_coroutine_system.stub.php b/ext-src/stubs/php_swoole_coroutine_system.stub.php
@@ -11,7 +11,7 @@ public static function readFile(string $filename, int $flag = 0): false|string {
         public static function writeFile(string $filename, string $fileContent, int $flags = 0): false|int {}
         public static function wait(float $timeout = -1): array|false {}
         public static function waitPid(int $pid, float $timeout = -1): array|false {}
-        public static function waitSignal(int|array $signals, float $timeout = -1): bool {}
+        public static function waitSignal(int|array $signals, float $timeout = -1): int|false {}
         public static function waitEvent(mixed $socket, int $events = SWOOLE_EVENT_READ, float $timeout = -1): int|false {}
     }
 }
diff --git a/ext-src/stubs/php_swoole_coroutine_system_arginfo.h b/ext-src/stubs/php_swoole_coroutine_system_arginfo.h
@@ -1,5 +1,5 @@
 /* This is a generated file, edit the .stub.php file instead.
- * Stub hash: 59e0a163279f6b7bf53de6f8f50a8ba4820ccf3b */
+ * Stub hash: 3c270ea28b44ea9ae57763943f8e0188d2fbcc03 */
 
 ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_class_Swoole_Coroutine_System_gethostbyname, 0, 1, MAY_BE_FALSE|MAY_BE_STRING)
 	ZEND_ARG_TYPE_INFO(0, domain_name, IS_STRING, 0)
@@ -55,7 +55,7 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_class_Swoole_Coroutine_System_wa
 	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, timeout, IS_DOUBLE, 0, "-1")
 ZEND_END_ARG_INFO()
 
-ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_Swoole_Coroutine_System_waitSignal, 0, 1, _IS_BOOL, 0)
+ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_class_Swoole_Coroutine_System_waitSignal, 0, 1, MAY_BE_LONG|MAY_BE_FALSE)
 	ZEND_ARG_TYPE_MASK(0, signals, MAY_BE_LONG|MAY_BE_ARRAY, NULL)
 	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, timeout, IS_DOUBLE, 0, "-1")
 ZEND_END_ARG_INFO()

diff --git a/ext-src/swoole_coroutine_system.cc b/ext-src/swoole_coroutine_system.cc
@@ -356,7 +356,8 @@ PHP_METHOD(swoole_coroutine_system, waitSignal) {
         signals.push_back(zval_get_long(zsignals));
     }
 
-    if (!System::wait_signal(signals, timeout)) {
+    int signo = System::wait_signal(signals, timeout);
+    if (signo == -1) {
         if (swoole_get_last_error() == EBUSY) {
             php_swoole_fatal_error(E_WARNING, "Unable to wait signal, async signal listener has been registered");
         } else if (swoole_get_last_error() == EINVAL) {
@@ -366,7 +367,7 @@ PHP_METHOD(swoole_coroutine_system, waitSignal) {
         RETURN_FALSE;
     }
 
-    RETURN_TRUE;
+    RETURN_LONG(signo);
 }
 
 PHP_METHOD(swoole_coroutine_system, waitEvent) {

diff --git a/include/swoole_coroutine_system.h b/include/swoole_coroutine_system.h
@@ -67,8 +67,8 @@ class System {
     static pid_t wait(int *__stat_loc, double timeout = -1);
     static pid_t waitpid(pid_t __pid, int *__stat_loc, int __options, double timeout = -1);
     /* signal */
-    static bool wait_signal(int signal, double timeout = -1);
-    static bool wait_signal(const std::vector<int> &signals, double timeout = -1);
+    static int wait_signal(int signal, double timeout = -1);
+    static int wait_signal(const std::vector<int> &signals, double timeout = -1);
     /* event */
     static int wait_event(int fd, int events, double timeout);
 };

diff --git a/src/coroutine/system.cc b/src/coroutine/system.cc
@@ -223,39 +223,53 @@ std::vector<std::string> System::getaddrinfo(
     return retval;
 }
 
-bool System::wait_signal(int signal, double timeout) {
+struct SignalListener {
+    Coroutine *co;
+    int signo;
+};
+
+/**
+ * Only the main thread should listen for signals,
+ * without modifying it to a thread-local variable.
+ */
+static SignalListener *listeners[SW_SIGNO_MAX];
+
+int System::wait_signal(int signal, double timeout) {
     std::vector<int> signals = {signal};
     return wait_signal(signals, timeout);
 }
 
 /**
  * @error: swoole_get_last_error()
  */
-bool System::wait_signal(const std::vector<int> &signals, double timeout) {
-    static Coroutine *listeners[SW_SIGNO_MAX];
-    Coroutine *co = Coroutine::get_current_safe();
+int System::wait_signal(const std::vector<int> &signals, double timeout) {
+    SignalListener listener = {
+            Coroutine::get_current_safe(),
+            -1,
+    };
 
     if (SwooleTG.signal_listener_num > 0) {
         swoole_set_last_error(EBUSY);
-        return false;
+        return -1;
     }
 
     auto callback_fn = [](int signo) {
-        Coroutine *co = listeners[signo];
-        if (co) {
+        auto listener = listeners[signo];
+        if (listener) {
             listeners[signo] = nullptr;
-            co->resume();
+            listener->signo = signo;
+            listener->co->resume();
         }
     };
 
     for (auto &signo : signals) {
         if (signo < 0 || signo >= SW_SIGNO_MAX || signo == SIGCHLD) {
             swoole_set_last_error(EINVAL);
-            return false;
+            return -1;
         }
 
         /* resgiter signal */
-        listeners[signo] = co;
+        listeners[signo] = &listener;
 
 #ifdef SW_USE_THREAD_CONTEXT
         swoole_event_defer([signo, &callback_fn](void *) { swoole_signal_set(signo, callback_fn); }, nullptr);
@@ -273,7 +287,7 @@ bool System::wait_signal(const std::vector<int> &signals, double timeout) {
 
     SwooleTG.co_signal_listener_num++;
 
-    bool retval = co->yield_ex(timeout);
+    bool retval = listener.co->yield_ex(timeout);
 
     for (auto &signo : signals) {
 #ifdef SW_USE_THREAD_CONTEXT
@@ -286,7 +300,7 @@ bool System::wait_signal(const std::vector<int> &signals, double timeout) {
 
     SwooleTG.co_signal_listener_num--;
 
-    return retval;
+    return retval ? listener.signo : -1;
 }
 
 struct CoroPollTask {

diff --git a/tests/swoole_coroutine_system/waitSignal.phpt b/tests/swoole_coroutine_system/waitSignal.phpt
@@ -32,13 +32,13 @@ Coroutine\run(function () use ($atomic) {
     switch_process();
     $atomic->wakeup();
     echo "1\n";
-    Assert::true(System::waitSignal(SIGUSR1));
+    Assert::eq(System::waitSignal(SIGUSR1), SIGUSR1);
     echo "3\n";
     Assert::false(System::waitSignal(SIGUSR2, 0.01));
     echo "4\n";
     $atomic->wakeup();
     echo "5\n";
-    Assert::true(System::waitSignal(SIGUSR2));
+    Assert::eq(System::waitSignal(SIGUSR2), SIGUSR2);
     echo "7\n";
     System::wait();
     echo "9\n";

diff --git a/tests/swoole_coroutine_system/waitSignal_2.phpt b/tests/swoole_coroutine_system/waitSignal_2.phpt
@@ -33,13 +33,13 @@ Coroutine\run(function () use ($atomic) {
     $atomic->wakeup();
     echo "1\n";
     $list = [SIGUSR1, SIGUSR2, SIGIO];
-    Assert::true(System::waitSignal($list));
+    Assert::eq(System::waitSignal($list), SIGUSR1);
     echo "3\n";
     Assert::false(System::waitSignal($list, 0.01));
     echo "4\n";
     $atomic->wakeup();
     echo "5\n";
-    Assert::true(System::waitSignal($list));
+    Assert::eq(System::waitSignal($list), SIGUSR2);
     echo "7\n";
     System::wait();
     echo "9\n";

diff --git a/tests/swoole_timer/bug_4794_4.phpt b/tests/swoole_timer/bug_4794_4.phpt
@@ -32,13 +32,13 @@ Coroutine\run(function () use ($atomic) {
     switch_process();
     $atomic->wakeup();
     echo "1\n";
-    Assert::true(System::waitSignal(SIGUSR1));
+    Assert::eq(System::waitSignal(SIGUSR1), SIGUSR1);
     echo "3\n";
     Assert::false(System::waitSignal(SIGUSR2, 0.0001));
     echo "4\n";
     $atomic->wakeup();
     echo "5\n";
-    Assert::true(System::waitSignal(SIGUSR2));
+    Assert::eq(System::waitSignal(SIGUSR2), SIGUSR2);
     echo "7\n";
     System::wait(0.0001);
     echo "9\n";