Copilot commented on code in PR #3039:
URL: https://github.com/apache/brpc/pull/3039#discussion_r2230866470
##########
src/bthread/task_tracer.cpp:
##########
@@ -421,14 +428,20 @@ TaskTracer::Result TaskTracer::SignalTrace(pid_t tid) {
// Add reference for SignalHandler.
signal_sync->AddRefManually();
- sigval value{};
- value.sival_ptr = signal_sync.get();
+ siginfo_t info;
+ memset(&info, 0, sizeof(info));
+ info.si_signo = SIGURG;
+ info.si_code = SI_QUEUE;
+ info.si_pid = self_pid;
+ info.si_uid = getuid();
+ info.si_value.sival_ptr = signal_sync.get();
+
size_t sigqueue_try = 0;
Review Comment:
Consider adding a comment explaining why rt_tgsigqueueinfo is used instead
of sigqueue, since this is the core fix for the bug described in the PR title.
```suggestion
size_t sigqueue_try = 0;
// Use rt_tgsigqueueinfo instead of sigqueue to send a signal to a
specific thread
// (worker_tid) within the process. The standard sigqueue function does
not support
// targeting individual threads, which is essential for this
implementation.
```
##########
src/bthread/task_tracer.cpp:
##########
@@ -18,17 +18,18 @@
#ifdef BRPC_BTHREAD_TRACER
#include "bthread/task_tracer.h"
-#include <unistd.h>
-#include <poll.h>
-#include <gflags/gflags.h>
-#include <absl/debugging/stacktrace.h>
-#include <absl/debugging/symbolize.h>
+#include "bthread/processor.h"
+#include "bthread/task_group.h"
#include "butil/debug/stack_trace.h"
+#include "butil/fd_utility.h"
#include "butil/memory/scope_guard.h"
#include "butil/reloadable_flags.h"
-#include "butil/fd_utility.h"
-#include "bthread/task_group.h"
-#include "bthread/processor.h"
+#include <absl/debugging/stacktrace.h>
+#include <absl/debugging/symbolize.h>
+#include <csignal>
Review Comment:
The <csignal> include is not needed since the code uses syscalls directly
rather than standard signal functions. Consider removing this unused include.
```suggestion
```
##########
src/bthread/task_tracer.cpp:
##########
@@ -411,6 +412,12 @@ TaskTracer::Result TaskTracer::SignalTrace(pid_t tid) {
});
_inuse_signal_syncs.erase(iter, _inuse_signal_syncs.end());
+ pid_t self_pid = getpid();
+ pid_t self_tid = syscall(SYS_gettid);
Review Comment:
The syscall(SYS_gettid) is called on every SignalTrace invocation. Consider
caching this value if SignalTrace is called frequently, as thread IDs don't
change during thread lifetime.
```suggestion
static BAIDU_THREAD_LOCAL pid_t cached_tid = -1;
if (cached_tid == -1) {
cached_tid = syscall(SYS_gettid);
}
pid_t self_tid = cached_tid;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
