Enhance socket binding check, print bound coroutine call stack in case of failure

Author: matyhtf

ext-src/php_swoole.cc

@@ -508,6 +508,14 @@ static void fatal_error(int code, const char *format, ...) {
 
     zend::print_error(exception, E_ERROR);
 
+    if (code == SW_ERROR_CO_HAS_BEEN_BOUND) {
+        fprintf(stderr, "\n [Coroutine-%ld] Stack trace:"
+               "\n -------------------------------------------------------------------"
+               "\n",
+               Coroutine::get_socket_bound_cid());
+        sw_php_print_backtrace(Coroutine::get_socket_bound_cid());
+    }
+
 #ifdef SW_THREAD
     if (!tsrm_is_main_thread()) {
         php_swoole_thread_bailout();
@@ -1145,7 +1153,7 @@ PHP_MINFO_FUNCTION(swoole) {
     php_info_print_table_row(2, "execinfo", "enabled");
 #endif
 #ifdef HAVE_SSH2LIB
-	 php_swoole_ssh2_minfo();
+    php_swoole_ssh2_minfo();
 #endif
     php_info_print_table_end();
 

include/swoole_coroutine.h

@@ -144,7 +144,7 @@ class Coroutine {
     static void set_on_close(SwapCallback func);
     static void bailout(const BailoutCallback &func);
 
-    static inline long create(const CoroutineFunc &fn, void *args = nullptr) {
+    static long create(const CoroutineFunc &fn, void *args = nullptr) {
 #ifdef SW_USE_THREAD_CONTEXT
         try {
             return (new Coroutine(fn, args))->run();
@@ -161,61 +161,65 @@ class Coroutine {
     static void activate();
     static void deactivate();
 
-    static inline Coroutine *get_current() {
+    static Coroutine *get_current() {
         return current;
     }
 
-    static inline Coroutine *get_current_safe() {
+    static Coroutine *get_current_safe() {
         if (sw_unlikely(!current)) {
             swoole_fatal_error(SW_ERROR_CO_OUT_OF_COROUTINE, "API must be called in the coroutine");
         }
         return current;
     }
 
-    static inline void *get_current_task() {
+    static void *get_current_task() {
         return sw_likely(current) ? current->get_task() : nullptr;
     }
 
-    static inline long get_current_cid() {
+    static long get_current_cid() {
         return sw_likely(current) ? current->get_cid() : -1;
     }
 
-    static inline Coroutine *get_by_cid(long cid) {
+    static Coroutine *get_by_cid(long cid) {
         auto i = coroutines.find(cid);
         return sw_likely(i != coroutines.end()) ? i->second : nullptr;
     }
 
-    static inline void *get_task_by_cid(long cid) {
+    static void *get_task_by_cid(long cid) {
         Coroutine *co = get_by_cid(cid);
         return sw_likely(co) ? co->get_task() : nullptr;
     }
 
-    static inline size_t get_stack_size() {
+    static size_t get_stack_size() {
         return stack_size;
     }
 
-    static inline void set_stack_size(size_t size) {
+    static void set_stack_size(size_t size) {
         stack_size = SW_MEM_ALIGNED_SIZE_EX(SW_MAX(MIN_STACK_SIZE, SW_MIN(size, MAX_STACK_SIZE)), STACK_ALIGNED_SIZE);
     }
 
-    static inline long get_last_cid() {
+    static long get_last_cid() {
         return last_cid;
     }
 
-    static inline size_t count() {
+    static long get_socket_bound_cid() {
+        return socket_bound_cid;
+    }
+
+    static size_t count() {
         return coroutines.size();
     }
 
-    static inline uint64_t get_peak_num() {
+    static uint64_t get_peak_num() {
         return peak_num;
     }
 
-    static inline long get_elapsed(long cid) {
+    static long get_elapsed(long cid) {
         Coroutine *co = cid == 0 ? get_current() : get_by_cid(cid);
         return sw_likely(co) ? Timer::get_absolute_msec() - co->get_init_msec() : -1;
     }
 
-    static inline long get_execute_time(long cid) {
+    static long get_execute_time(long cid) {
         Coroutine *co = cid == 0 ? get_current() : get_by_cid(cid);
         return sw_likely(co) ? co->get_execute_usec() : -1;
     }
@@ -224,10 +228,12 @@ class Coroutine {
     static void calc_execute_usec(Coroutine *yield_coroutine, Coroutine *resume_coroutine);
 #endif
     static void print_list();
+    static void print_socket_bound_error(int sock_fd, const char *event_str, long bound_cid);
 
   protected:
     static SW_THREAD_LOCAL Coroutine *current;
     static SW_THREAD_LOCAL long last_cid;
+    static SW_THREAD_LOCAL long socket_bound_cid;
     static SW_THREAD_LOCAL uint64_t peak_num;
     static SW_THREAD_LOCAL size_t stack_size;
     static SW_THREAD_LOCAL SwapCallback on_yield;      /* before yield */

include/swoole_coroutine_socket.h

@@ -305,15 +305,9 @@ class Socket {
     const char *get_event_str(EventType event) const;
 
     void check_bound_co(const EventType event) const {
-        long cid = get_bound_cid(event);
-        if (sw_unlikely(cid)) {
-            swoole_fatal_error(SW_ERROR_CO_HAS_BEEN_BOUND,
-                               "Socket#%d has already been bound to another coroutine#%ld, "
-                               "%s of the same socket in coroutine#%ld at the same time is not allowed",
-                               sock_fd,
-                               cid,
-                               get_event_str(event),
-                               Coroutine::get_current_cid());
+        auto bound_cid = get_bound_cid(event);
+        if (sw_unlikely(bound_cid)) {
+            Coroutine::print_socket_bound_error(sock_fd, get_event_str(event), bound_cid);
         }
     }
 

src/coroutine/base.cc

@@ -21,6 +21,7 @@ namespace swoole {
 
 SW_THREAD_LOCAL Coroutine *Coroutine::current = nullptr;
 SW_THREAD_LOCAL long Coroutine::last_cid = 0;
+SW_THREAD_LOCAL long Coroutine::socket_bound_cid = 0;
 SW_THREAD_LOCAL std::unordered_map<long, Coroutine *> Coroutine::coroutines;
 SW_THREAD_LOCAL uint64_t Coroutine::peak_num = 0;
 SW_THREAD_LOCAL bool Coroutine::activated = false;
@@ -221,6 +222,17 @@ void Coroutine::print_list() {
     }
 }
 
+void Coroutine::print_socket_bound_error(int sock_fd, const char *event_str, long bound_cid) {
+    socket_bound_cid = bound_cid;
+    swoole_fatal_error(SW_ERROR_CO_HAS_BEEN_BOUND,
+                       "Socket#%d has already been bound to another coroutine#%ld, "
+                       "%s of the same socket in coroutine#%ld at the same time is not allowed",
+                       sock_fd,
+                       socket_bound_cid,
+                       event_str,
+					   get_current_cid());
+}
+
 void Coroutine::set_on_yield(const SwapCallback func) {
     on_yield = func;
 }

tests/swoole_socket_coro/bound_error.phpt

@@ -0,0 +1,36 @@
+--TEST--
+swoole_socket_coro: bound error
+--SKIPIF--
+<?php require __DIR__ . '/../include/skipif.inc'; ?>
+--FILE--
+<?php
+require __DIR__ . '/../include/bootstrap.php';
+$port = get_one_free_port();
+go(function () use ($port) {
+    $server = new Co\Socket(AF_INET, SOCK_STREAM, IPPROTO_IP);
+    Assert::assert($server->bind('127.0.0.1', $port));
+    Assert::assert($server->listen());
+});
+go(function () use ($port)  {
+    $cli = new Swoole\Coroutine\Client(SWOOLE_SOCK_TCP);
+    $ret = $cli->connect('127.0.0.1', $port);
+    Assert::true($ret);
+    go(function () use ($cli) {
+        $cli->recv();
+    });
+    $cli->recv();
+});
+Swoole\Event::wait();
+?>
+--EXPECTF--
+Fatal error: Uncaught Swoole\Error: Socket#%d has already been bound to another coroutine#%d, reading of the same socket in coroutine#%d at the same time is not allowed in %s:%d
+Stack trace:
+#0 %s(%d): Swoole\Coroutine\Client->recv()
+#1 [internal function]: {closure:%s:%d}()
+#2 {main}
+  thrown in %s on line %d
+
+ [Coroutine-3] Stack trace:
+ -------------------------------------------------------------------
+#0 %s(%d): Swoole\Coroutine\Client->recv()
+#1 [internal function]: {closure:{%s:%d}:%d}()