Refactor AsyncRequest, fix #5547 (#5550)

* Refactor AsyncRequest, fix #5547 * fix * add tests * optimize code * optimize tests * optimize code 2
swoole · Oct 29, 2024 · 741e352 · 741e352
1 parent 3016130
commit 741e352
Show file tree

Hide file tree

Showing 14 changed files with 118 additions and 77 deletions.
diff --git a/core-tests/src/network/dns.cpp b/core-tests/src/network/dns.cpp
@@ -74,14 +74,7 @@ TEST(dns, cancel) {
 }
 
 TEST(dns, getaddrinfo) {
-    char buf[1024] = {};
-    swoole::network::GetaddrinfoRequest req = {};
-    req.hostname = "www.baidu.com";
-    req.family = AF_INET;
-    req.socktype = SOCK_STREAM;
-    req.protocol = 0;
-    req.service = nullptr;
-    req.result = buf;
+    swoole::GetaddrinfoRequest req("www.baidu.com", AF_INET, SOCK_STREAM, 0, "");
     ASSERT_EQ(swoole::network::getaddrinfo(&req), 0);
     ASSERT_GT(req.count, 0);
 

diff --git a/include/swoole_async.h b/include/swoole_async.h
@@ -39,6 +39,10 @@ enum AsyncFlag {
     SW_AIO_EOF = 1u << 2,
 };
 
+struct AsyncRequest {
+    virtual ~AsyncRequest() = default;
+};
+
 struct AsyncEvent {
     size_t task_id;
 #ifdef SW_USE_IOURING
@@ -49,7 +53,7 @@ struct AsyncEvent {
     /**
      * input & output
      */
-    void *data;
+    std::shared_ptr<AsyncRequest> data;
 #ifdef SW_USE_IOURING
     const char *pathname;
     const char *pathname2;
@@ -81,22 +85,47 @@ struct AsyncEvent {
     }
 };
 
-struct GethostbynameRequest {
-    const char *name;
+struct GethostbynameRequest : public AsyncRequest {
+    std::string name;
     int family;
     char *addr;
     size_t addr_len;
 
-    GethostbynameRequest(const char *_name, int _family) : name(_name), family(_family) {
+    GethostbynameRequest(std::string _name, int _family) : name(std::move(_name)), family(_family) {
         addr_len = _family == AF_INET6 ? INET6_ADDRSTRLEN : INET_ADDRSTRLEN;
         addr = new char[addr_len];
     }
 
-    ~GethostbynameRequest() {
+    ~GethostbynameRequest() override {
         delete[] addr;
     }
 };
 
+struct GetaddrinfoRequest : public AsyncRequest {
+    std::string hostname;
+    std::string service;
+    int family;
+    int socktype;
+    int protocol;
+    int error;
+    std::vector<struct sockaddr_in6> results;
+    int count;
+
+    void parse_result(std::vector<std::string> &retval);
+
+    GetaddrinfoRequest(std::string _hostname, int _family, int _socktype, int _protocol, std::string _service)
+        : hostname(std::move(_hostname)),
+          service(std::move(_service)) {
+        family =_family;
+        socktype =_socktype;
+        protocol =_protocol;
+        count = 0;
+        error = 0;
+    }
+
+    ~GetaddrinfoRequest() override = default;
+};
+
 class AsyncThreads {
   public:
     size_t task_num = 0;

diff --git a/include/swoole_socket.h b/include/swoole_socket.h
@@ -64,20 +64,9 @@ enum {
 };
 
 namespace swoole {
-namespace network {
+struct GetaddrinfoRequest;
 
-struct GetaddrinfoRequest {
-    const char *hostname;
-    const char *service;
-    int family;
-    int socktype;
-    int protocol;
-    int error;
-    void *result;
-    int count;
-
-    void parse_result(std::vector<std::string> &retval);
-};
+namespace network {
 
 struct SendfileTask {
     off_t offset;

diff --git a/include/swoole_util.h b/include/swoole_util.h
@@ -153,7 +153,7 @@ class BitMap {
     }
 
     void clear() {
-        memset(array_, 0, get_array_size(n_bits_));
+        memset(array_, 0, sizeof(uint64_t) * get_array_size(n_bits_));
     }
 
     void set(size_t i) {

diff --git a/scripts/make.sh b/scripts/make.sh
@@ -1,16 +1,20 @@
-#!/bin/sh -e
+#!/bin/sh
 __CURRENT_DIR__=$(cd "$(dirname "$0")";pwd)
 __DIR__=$(cd "$(dirname "${__CURRENT_DIR__}")";pwd)
+__HAVE_ZTS__=$(php -v|grep ZTS)
+
 COMPILE_PARAMS="--enable-openssl \
 --enable-sockets \
 --enable-mysqlnd \
 --enable-swoole-curl \
 --enable-cares \
---enable-swoole-thread \
 --enable-swoole-pgsql \
 --with-swoole-odbc=unixODBC,/usr \
 --enable-swoole-sqlite"
 
+if [ -n "$__HAVE_ZTS__" ]; then
+    COMPILE_PARAMS="$COMPILE_PARAMS --enable-swoole-thread"
+fi
 
 if [ "$(uname | grep -i darwin)"x != ""x ]; then
   CPU_COUNT="$(sysctl -n machdep.cpu.core_count)"

diff --git a/src/coroutine/system.cc b/src/coroutine/system.cc
@@ -137,8 +137,8 @@ ssize_t System::write_file(const char *file, char *buf, size_t length, bool lock
 
 std::string gethostbyname_impl_with_async(const std::string &hostname, int domain, double timeout) {
     AsyncEvent ev{};
-    GethostbynameRequest dns_request(hostname.c_str(), domain);
-    ev.data = &dns_request;
+    auto req = new GethostbynameRequest(hostname, domain);
+    ev.data = std::shared_ptr<AsyncRequest>(req);
     ev.retval = 1;
 
     coroutine::async(async::handler_gethostbyname, ev, timeout);
@@ -150,7 +150,7 @@ std::string gethostbyname_impl_with_async(const std::string &hostname, int domai
         swoole_set_last_error(ev.error);
         return "";
     } else {
-        std::string addr(dns_request.addr);
+        std::string addr(req->addr);
         return addr;
     }
 }
@@ -199,30 +199,20 @@ std::vector<std::string> System::getaddrinfo(
     assert(family == AF_INET || family == AF_INET6);
 
     AsyncEvent ev{};
-    network::GetaddrinfoRequest req{};
-
-    ev.data = &req;
-
-    struct sockaddr_in6 result_buffer[SW_DNS_HOST_BUFFER_SIZE];
-
-    req.hostname = hostname.c_str();
-    req.family = family;
-    req.socktype = socktype;
-    req.protocol = protocol;
-    req.service = service.empty() ? nullptr : service.c_str();
-    req.result = result_buffer;
+    auto req = new GetaddrinfoRequest(hostname, family, socktype, protocol, service);
+    ev.data = std::shared_ptr<AsyncRequest>(req);
 
     coroutine::async(async::handler_getaddrinfo, ev, timeout);
 
     std::vector<std::string> retval;
 
-    if (ev.retval == -1 || req.error != 0) {
+    if (ev.retval == -1 || req->error != 0) {
         if (ev.error == SW_ERROR_AIO_TIMEOUT) {
             ev.error = SW_ERROR_DNSLOOKUP_RESOLVE_TIMEOUT;
         }
         swoole_set_last_error(ev.error);
     } else {
-        req.parse_result(retval);
+        req->parse_result(retval);
     }
 
     return retval;

diff --git a/src/network/client.cc b/src/network/client.cc
@@ -615,14 +615,13 @@ static int Client_tcp_connect_async(Client *cli, const char *host, int port, dou
 
     if (cli->wait_dns) {
         AsyncEvent ev{};
-        auto dns_request = new GethostbynameRequest(cli->server_host, cli->_sock_domain);
-        ev.data = dns_request;
+        auto req = new GethostbynameRequest(cli->server_host, cli->_sock_domain);
+        ev.data = std::shared_ptr<AsyncRequest>(req);
         ev.object = cli;
         ev.handler = async::handler_gethostbyname;
         ev.callback = Client_onResolveCompleted;
 
         if (swoole::async::dispatch(&ev) == nullptr) {
-            delete dns_request;
             return SW_ERR;
         } else {
             return SW_OK;
@@ -1112,17 +1111,13 @@ static void Client_onTimeout(Timer *timer, TimerNode *tnode) {
 }
 
 static void Client_onResolveCompleted(AsyncEvent *event) {
-    auto dns_request = (GethostbynameRequest *) event->data;
-    if (event->canceled) {
-        delete dns_request;
-        return;
-    }
+    GethostbynameRequest *req = dynamic_cast<GethostbynameRequest *>(event->data.get());
 
     Client *cli = (Client *) event->object;
     cli->wait_dns = 0;
 
     if (event->error == 0) {
-        Client_tcp_connect_async(cli, dns_request->addr, cli->server_port, cli->timeout, 1);
+        Client_tcp_connect_async(cli, req->addr, cli->server_port, cli->timeout, 1);
     } else {
         swoole_set_last_error(SW_ERROR_DNSLOOKUP_RESOLVE_FAILED);
         cli->socket->removed = 1;
@@ -1131,7 +1126,6 @@ static void Client_onResolveCompleted(AsyncEvent *event) {
             cli->onError(cli);
         }
     }
-    delete dns_request;
 }
 
 static int Client_onWrite(Reactor *reactor, Event *event) {

diff --git a/src/network/dns.cc b/src/network/dns.cc
@@ -344,7 +344,7 @@ std::vector<std::string> dns_lookup_impl_with_socket(const char *domain, int fam
         temp = &packet[steps];
         j = 0;
         while (*temp != 0) {
-            if ((uchar)(*temp) == 0xc0) {
+            if ((uchar) (*temp) == 0xc0) {
                 ++temp;
                 temp = &packet[(uint8_t) *temp];
             } else {
@@ -373,7 +373,7 @@ std::vector<std::string> dns_lookup_impl_with_socket(const char *domain, int fam
             temp = &packet[steps];
             j = 0;
             while (*temp != 0) {
-                if ((uchar)(*temp) == 0xc0) {
+                if ((uchar) (*temp) == 0xc0) {
                     ++temp;
                     temp = &packet[(uint8_t) *temp];
                 } else {
@@ -767,36 +767,40 @@ int getaddrinfo(GetaddrinfoRequest *req) {
     hints.ai_socktype = req->socktype;
     hints.ai_protocol = req->protocol;
 
-    int ret = ::getaddrinfo(req->hostname, req->service, &hints, &result);
+    int ret = ::getaddrinfo(req->hostname.c_str(), req->service.c_str(), &hints, &result);
     if (ret != 0) {
         req->error = ret;
         return SW_ERR;
     }
 
-    void *buffer = req->result;
     int i = 0;
-    for (ptr = result; ptr != nullptr; ptr = ptr->ai_next) {
+    for (ptr = result; ptr != nullptr; ptr = ptr->ai_next, i++) {
+    }
+    req->count = SW_MIN(i, SW_DNS_HOST_BUFFER_SIZE);
+    req->results.resize(req->count);
+
+    for (ptr = result, i = 0; ptr != nullptr; ptr = ptr->ai_next, i++) {
         switch (ptr->ai_family) {
         case AF_INET:
-            memcpy((char *) buffer + (i * sizeof(struct sockaddr_in)), ptr->ai_addr, sizeof(struct sockaddr_in));
+            memcpy(&req->results[i], ptr->ai_addr, sizeof(struct sockaddr_in));
             break;
         case AF_INET6:
-            memcpy((char *) buffer + (i * sizeof(struct sockaddr_in6)), ptr->ai_addr, sizeof(struct sockaddr_in6));
+            memcpy(&req->results[i], ptr->ai_addr, sizeof(struct sockaddr_in6));
             break;
         default:
             swoole_warning("unknown socket family[%d]", ptr->ai_family);
             break;
         }
-        i++;
         if (i == SW_DNS_HOST_BUFFER_SIZE) {
             break;
         }
     }
     ::freeaddrinfo(result);
     req->error = 0;
-    req->count = i;
+
     return SW_OK;
 }
+}  // namespace network
 
 void GetaddrinfoRequest::parse_result(std::vector<std::string> &retval) {
     struct sockaddr_in *addr_v4;
@@ -805,18 +809,17 @@ void GetaddrinfoRequest::parse_result(std::vector<std::string> &retval) {
     char tmp[INET6_ADDRSTRLEN];
     const char *r;
 
-    for (int i = 0; i < count; i++) {
+    for (auto &addr : results) {
         if (family == AF_INET) {
-            addr_v4 = (struct sockaddr_in *) ((char *) result + (i * sizeof(struct sockaddr_in)));
+            addr_v4 = (struct sockaddr_in *) &addr;
             r = inet_ntop(AF_INET, (const void *) &addr_v4->sin_addr, tmp, sizeof(tmp));
         } else {
-            addr_v6 = (struct sockaddr_in6 *) ((char *) result + (i * sizeof(struct sockaddr_in6)));
+            addr_v6 = (struct sockaddr_in6 *) &addr;
             r = inet_ntop(AF_INET6, (const void *) &addr_v6->sin6_addr, tmp, sizeof(tmp));
         }
         if (r) {
             retval.push_back(tmp);
         }
     }
 }
-}  // namespace network
 }  // namespace swoole
diff --git a/src/os/base.cc b/src/os/base.cc
@@ -93,14 +93,14 @@ namespace async {
 
 void handler_gethostbyname(AsyncEvent *event) {
     char addr[INET6_ADDRSTRLEN];
-    auto request = (GethostbynameRequest *) event->data;
-    int ret = network::gethostbyname(request->family, request->name, addr);
-    sw_memset_zero(request->addr, request->addr_len);
+    auto req = dynamic_cast<GethostbynameRequest *>(event->data.get());
+    int ret = network::gethostbyname(req->family, req->name.c_str(), addr);
+    sw_memset_zero(req->addr, req->addr_len);
 
     if (ret < 0) {
         event->error = SW_ERROR_DNSLOOKUP_RESOLVE_FAILED;
     } else {
-        if (inet_ntop(request->family, addr, request->addr, request->addr_len) == nullptr) {
+        if (inet_ntop(req->family, addr, req->addr, req->addr_len) == nullptr) {
             ret = -1;
             event->error = SW_ERROR_BAD_IPV6_ADDRESS;
         } else {
@@ -112,7 +112,7 @@ void handler_gethostbyname(AsyncEvent *event) {
 }
 
 void handler_getaddrinfo(AsyncEvent *event) {
-    network::GetaddrinfoRequest *req = (network::GetaddrinfoRequest *) event->data;
+    auto req = dynamic_cast<GetaddrinfoRequest *>(event->data.get());
     event->retval = network::getaddrinfo(req);
     event->error = req->error;
 }

diff --git a/tests/include/functions.php b/tests/include/functions.php
@@ -865,3 +865,10 @@ function get_thread_name(): string
 {
     return trim(file_get_contents('/proc/' . posix_getpid() . '/task/' . \Swoole\Thread::getNativeId() . '/comm'));
 }
+
+function mkdir_if_not_exists(string $string): void
+{
+    if (!is_dir($string)) {
+        mkdir($string, 0777, true);
+    }
+}
diff --git a/tests/include/skipif.inc b/tests/include/skipif.inc
@@ -45,7 +45,7 @@ function swoole_color(string $content, int $color): string
 function skip(string $reason, bool $is_skip = true, int $color = SWOOLE_COLOR_YELLOW)
 {
     if ($is_skip) {
-        exit('skip ' . swoole_color($reason, $color));
+        exit('skip ' . swoole_color($reason, $color) . "\n");
     }
 }
 

diff --git a/tests/swoole_coroutine_system/getaddrinfo.phpt b/tests/swoole_coroutine_system/getaddrinfo.phpt
@@ -7,7 +7,7 @@ skip_if_offline();
 --FILE--
 <?php
 require __DIR__ . '/../include/bootstrap.php';
-go(function () {
+Co\run(function () {
     $ip_list = Swoole\Coroutine\System::getaddrinfo('www.baidu.com', AF_INET);
     Assert::assert(!empty($ip_list) and is_array($ip_list));
     foreach ($ip_list as $ip) {