Refactor the implementation of runtime hooks under ZTS mode, Fix thre…

…ad safety issues. (#5617) * refactor runtime hook with ZTS * fix * optimize code * fix type * fix tests * fix tests [2] * fix * fix * fix 4 * fix tests * optimize code, add core tests
swoole · Dec 13, 2024 · b3c61e1 · b3c61e1
1 parent 528ede0
commit b3c61e1
Show file tree

Hide file tree

Showing 18 changed files with 341 additions and 219 deletions.
diff --git a/core-tests/src/coroutine/system.cpp b/core-tests/src/coroutine/system.cpp
@@ -269,3 +269,12 @@ TEST(coroutine_system, timeout_is_zero) {
         ASSERT_TRUE(result);
     });
 }
+
+TEST(coroutine_system, exec) {
+    test::coroutine::run([](void *arg) {
+        int status;
+        auto buffer = std::shared_ptr<String>(swoole::make_string(1024));
+        ASSERT_TRUE(System::exec("ls /", true, buffer, &status));
+        ASSERT_TRUE(buffer->contains(SW_STRL("tmp")));
+    });
+}
diff --git a/core-tests/src/os/wait.cpp b/core-tests/src/os/wait.cpp
@@ -2,17 +2,34 @@
 
 using namespace swoole;
 using namespace swoole::test;
+using swoole::coroutine::System;
 
-TEST(os_wait, waitpid_before_child_exit) {
-    test::coroutine::run([](void *arg) {
-        pid_t pid = fork();
-        ASSERT_NE(pid, -1);
+static pid_t fork_child() {
+    pid_t pid = fork();
+    EXPECT_NE(pid, -1);
 
-        if (pid == 0) {
-            usleep(100000);
-            exit(0);
-        }
+    if (pid == 0) {
+        usleep(100000);
+        exit(0);
+    }
+    return pid;
+}
+
+static pid_t fork_child2() {
+    pid_t pid = fork();
+    EXPECT_NE(pid, -1);
 
+    if (pid == 0) {
+        exit(0);
+    }
+
+    usleep(100000);
+    return pid;
+}
+
+TEST(os_wait, waitpid_before_child_exit) {
+    test::coroutine::run([](void *arg) {
+        auto pid = fork_child();
         int status = -1;
         pid_t pid2 = swoole_coroutine_waitpid(pid, &status, 0);
         ASSERT_EQ(status, 0);
@@ -22,14 +39,7 @@ TEST(os_wait, waitpid_before_child_exit) {
 
 TEST(os_wait, waitpid_after_child_exit) {
     test::coroutine::run([](void *arg) {
-        pid_t pid = fork();
-        ASSERT_NE(pid, -1);
-
-        if (pid == 0) {
-            exit(0);
-        }
-
-        usleep(100000);
+        pid_t pid = fork_child2();
         int status = -1;
         pid_t pid2 = swoole_coroutine_waitpid(pid, &status, 0);
         ASSERT_EQ(status, 0);
@@ -39,14 +49,7 @@ TEST(os_wait, waitpid_after_child_exit) {
 
 TEST(os_wait, wait_before_child_exit) {
     test::coroutine::run([](void *arg) {
-        pid_t pid = fork();
-        ASSERT_NE(pid, -1);
-
-        if (pid == 0) {
-            usleep(100000);
-            exit(0);
-        }
-
+        pid_t pid = fork_child();
         int status = -1;
         pid_t pid2 = -1;
 
@@ -63,14 +66,7 @@ TEST(os_wait, wait_before_child_exit) {
 
 TEST(os_wait, wait_after_child_exit) {
     test::coroutine::run([](void *arg) {
-        pid_t pid = fork();
-        ASSERT_NE(pid, -1);
-
-        if (pid == 0) {
-            exit(0);
-        }
-
-        usleep(100000);
+        pid_t pid = fork_child2();
         int status = -1;
         pid_t pid2 = -1;
 
@@ -84,3 +80,14 @@ TEST(os_wait, wait_after_child_exit) {
         ASSERT_EQ(WEXITSTATUS(status), 0);
     });
 }
+
+TEST(os_wait, waitpid_safe) {
+    test::coroutine::run([](void *arg) {
+        pid_t pid = fork_child2();
+        int status = -1;
+
+        pid_t pid2 = System::waitpid_safe(pid, &status, 0);
+        ASSERT_EQ(pid2, pid);
+        ASSERT_EQ(WEXITSTATUS(status), 0);
+    });
+}
diff --git a/ext-src/php_swoole.cc b/ext-src/php_swoole.cc
@@ -1551,6 +1551,14 @@ static PHP_FUNCTION(swoole_implicit_fn) {
         abort();
     } else if (SW_STRCASEEQ(fn, l_fn, "refcount")) {
         RETURN_LONG(zval_refcount_p(zargs));
+    } else if (SW_STRCASEEQ(fn, l_fn, "func_handler")) {
+        auto fn = zval_get_string(zargs);
+        zend_function *zf = (zend_function *) zend_hash_find_ptr(EG(function_table), fn);
+        zend_string_release(fn);
+        if (zf == nullptr) {
+            RETURN_FALSE;
+        }
+        printf("zif_handler=%p\n", zf->internal_function.handler);
     } else {
         zend_throw_exception_ex(swoole_exception_ce, SW_ERROR_INVALID_PARAMS, "unknown fn '%s'", fn);
     }

diff --git a/ext-src/php_swoole_cxx.h b/ext-src/php_swoole_cxx.h
@@ -638,40 +638,61 @@ class Callable {
     }
 };
 
-template<typename KeyT, typename ValueT>
+#define _CONCURRENCY_HASHMAP_LOCK_(code)                                                                               \
+    if (locked_) {                                                                                                     \
+        code;                                                                                                          \
+    } else {                                                                                                           \
+        lock_.lock();                                                                                                  \
+        code;                                                                                                          \
+        lock_.unlock();                                                                                                \
+    }
+
+template <typename KeyT, typename ValueT>
 class ConcurrencyHashMap {
- private:
+  private:
     std::unordered_map<KeyT, ValueT> map_;
     std::mutex lock_;
+    bool locked_;
     ValueT default_value_;
 
- public:
-    ConcurrencyHashMap(ValueT _default_value): map_(), lock_() {
+  public:
+    ConcurrencyHashMap(ValueT _default_value) : map_(), lock_() {
         default_value_ = _default_value;
+        locked_ = false;
     }
 
     void set(const KeyT &key, const ValueT &value) {
-        std::unique_lock<std::mutex> _lock(lock_);
-        map_[key] = value;
+        _CONCURRENCY_HASHMAP_LOCK_(map_[key] = value);
     }
 
     ValueT get(const KeyT &key) {
-        std::unique_lock<std::mutex> _lock(lock_);
-        auto iter = map_.find(key);
-        if (iter == map_.end()) {
-            return default_value_;
-        }
-        return iter->second;
+        ValueT value;
+        auto fn = [&]() -> ValueT {
+            auto iter = map_.find(key);
+            if (iter == map_.end()) {
+                return default_value_;
+            }
+            return iter->second;
+        };
+        _CONCURRENCY_HASHMAP_LOCK_(value = fn());
+        return value;
     }
 
     void del(const KeyT &key) {
-        std::unique_lock<std::mutex> _lock(lock_);
-        map_.erase(key);
+        _CONCURRENCY_HASHMAP_LOCK_(map_.erase(key));
     }
 
     void clear() {
+        _CONCURRENCY_HASHMAP_LOCK_(map_.clear());
+    }
+
+    void each(const std::function<void(KeyT key, ValueT value)> &cb) {
         std::unique_lock<std::mutex> _lock(lock_);
-        map_.clear();
+        locked_ = true;
+        for (auto &iter : map_) {
+            cb(iter.first, iter.second);
+        }
+        locked_ = false;
     }
 };
 

diff --git a/ext-src/stubs/php_swoole_runtime.stub.php b/ext-src/stubs/php_swoole_runtime.stub.php
@@ -1,7 +1,7 @@
 <?php
 namespace Swoole {
 	class Runtime {
-		public static function enableCoroutine(bool|int $enable = SWOOLE_HOOK_ALL, int $flags = SWOOLE_HOOK_ALL): bool {}
+		public static function enableCoroutine(int $flags = SWOOLE_HOOK_ALL): bool {}
 		public static function getHookFlags(): int {}
 		public static function setHookFlags(int $flags): bool {}
 	}

diff --git a/ext-src/stubs/php_swoole_runtime_arginfo.h b/ext-src/stubs/php_swoole_runtime_arginfo.h
@@ -1,8 +1,7 @@
 /* This is a generated file, edit the .stub.php file instead.
- * Stub hash: 608f0058d657d34cb5e9b5b9f3a31feaa105b294 */
+ * Stub hash: c42e4c108dd49f37a5953391a69cdad50106f17d */
 
 ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_Swoole_Runtime_enableCoroutine, 0, 0, _IS_BOOL, 0)
-	ZEND_ARG_TYPE_MASK(0, enable, MAY_BE_BOOL|MAY_BE_LONG, "SWOOLE_HOOK_ALL")
 	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, flags, IS_LONG, 0, "SWOOLE_HOOK_ALL")
 ZEND_END_ARG_INFO()
 

diff --git a/ext-src/swoole_coroutine_system.cc b/ext-src/swoole_coroutine_system.cc
@@ -236,56 +236,24 @@ PHP_METHOD(swoole_coroutine_system, exec) {
     Z_PARAM_BOOL(get_error_stream)
     ZEND_PARSE_PARAMETERS_END_EX(RETURN_FALSE);
 
-    if (php_swoole_signal_isset_handler(SIGCHLD)) {
-        php_swoole_error(E_WARNING, "The signal [SIGCHLD] is registered, cannot execute swoole_coroutine_exec");
-        RETURN_FALSE;
-    }
-
-    Coroutine::get_current_safe();
-
-    pid_t pid;
-    int fd = swoole_shell_exec(command, &pid, get_error_stream);
-    if (fd < 0) {
-        php_swoole_error(E_WARNING, "Unable to execute '%s'", command);
+    int status;
+    auto buffer = std::shared_ptr<String>(swoole::make_string(1024, sw_zend_string_allocator()));
+    if (!System::exec(command, get_error_stream, buffer, &status)) {
         RETURN_FALSE;
     }
 
-    String *buffer = new String(1024);
-    Socket socket(fd, SW_SOCK_UNIX_STREAM);
-    while (1) {
-        ssize_t retval = socket.read(buffer->str + buffer->length, buffer->size - buffer->length);
-        if (retval > 0) {
-            buffer->length += retval;
-            if (buffer->length == buffer->size) {
-                if (!buffer->extend()) {
-                    break;
-                }
-            }
-        } else {
-            break;
-        }
-    }
-    socket.close();
+    auto str = zend::fetch_zend_string_by_val(buffer->str);
+    buffer->set_null_terminated();
+    str->len = buffer->length;
+    buffer->release();
 
     zval zdata;
-    if (buffer->length == 0) {
-        ZVAL_EMPTY_STRING(&zdata);
-    } else {
-        ZVAL_STRINGL(&zdata, buffer->str, buffer->length);
-    }
-    delete buffer;
+    ZVAL_STR(&zdata, str);
 
-    int status;
-    pid_t _pid = swoole_coroutine_waitpid(pid, &status, 0);
-    if (_pid > 0) {
-        array_init(return_value);
-        add_assoc_long(return_value, "code", WEXITSTATUS(status));
-        add_assoc_long(return_value, "signal", WTERMSIG(status));
-        add_assoc_zval(return_value, "output", &zdata);
-    } else {
-        zval_ptr_dtor(&zdata);
-        RETVAL_FALSE;
-    }
+    array_init(return_value);
+    add_assoc_long(return_value, "code", WEXITSTATUS(status));
+    add_assoc_long(return_value, "signal", WTERMSIG(status));
+    add_assoc_zval(return_value, "output", &zdata);
 }
 
 static void swoole_coroutine_system_wait(INTERNAL_FUNCTION_PARAMETERS, pid_t pid, double timeout) {
@@ -310,6 +278,7 @@ static void swoole_coroutine_system_wait(INTERNAL_FUNCTION_PARAMETERS, pid_t pid
 }
 
 PHP_METHOD(swoole_coroutine_system, wait) {
+    SW_MUST_BE_MAIN_THREAD();
     double timeout = -1;
 
     ZEND_PARSE_PARAMETERS_START(0, 1)
@@ -321,6 +290,7 @@ PHP_METHOD(swoole_coroutine_system, wait) {
 }
 
 PHP_METHOD(swoole_coroutine_system, waitPid) {
+    SW_MUST_BE_MAIN_THREAD();
     zend_long pid;
     double timeout = -1;