Copilot commented on code in PR #3019:
URL: https://github.com/apache/brpc/pull/3019#discussion_r2188406550
##########
test/bthread_unittest.cpp:
##########
@@ -639,22 +638,18 @@ void spin_and_log_trace() {
while (!start) {
usleep(10 * 1000);
}
- bthread::FLAGS_enable_fast_unwind = false;
+
std::string st1 = bthread::stack_trace(th);
LOG(INFO) << "spin_and_log stack trace:\n" << st1;
+ ok = st1.find("spin_and_log") != std::string::npos;
- bthread::FLAGS_enable_fast_unwind = true;
- std::string st2 = bthread::stack_trace(th);
- LOG(INFO) << "fast_unwind spin_and_log stack trace:\n" << st2;
stop = true;
ASSERT_EQ(0, bthread_join(th, NULL));
- std::string st3 = bthread::stack_trace(th);
- LOG(INFO) << "ended bthread stack trace:\n" << st3;
- ASSERT_NE(std::string::npos, st3.find("not exist now"));
+ std::string st2 = bthread::stack_trace(th);
+ LOG(INFO) << "ended bthread stack trace:\n" << st2;
+ ASSERT_NE(std::string::npos, st2.find("not exist now"));
Review Comment:
The `ok` variable only checks `st1` but never includes `st2`. If `st2` fails
to contain "not exist now", the assertion will fire but the loop break will
still depend solely on `st1`. Consider updating `ok` to require both traces
succeed before breaking.
##########
src/bthread/task_tracer.cpp:
##########
@@ -472,99 +403,69 @@ TaskTracer::Result TaskTracer::SignalTrace(pid_t tid) {
// #19 0x00007f0d390088dc in __cxa_throw () from
/lib/x86_64-linux-gnu/libstdc++.so.6
// #20 0x00007f0d3b5b2245 in __cxxabiv1::__cxa_throw
(thrownException=0x7f0d114ea8c0, type=0x7f0d3d6dd830 <typeinfo for
rockset::GRPCError>, destructor=<optimized out>) at
/src/folly/folly/experimental/exception_tracer/ExceptionTracerLib.cpp:107
//
- // Therefore, we do not capture backtracks in the signal handler to avoid
mutex
- // reentry and deadlock. Instead, we capture backtracks in this function
and
- // ends the signal handler after capturing backtraces is complete.
- // Even so, there is still a deadlock problem:
- // the worker thread is interrupted when during an existing
dl_iterate_phdr call,
- // and wait for the capturing backtraces to complete. This function capture
- // backtracks with dl_iterate_phdr. We introduce a timeout mechanism in
signal
- // handler to avoid deadlock.
+ // Therefore, use async-signal-safe absl::DefaultStackUnwinder instead of
libunwind.
+
+ // Remove unused SignalSyncs.
+ auto iter = std::remove_if(
+ _inuse_signal_syncs.begin(), _inuse_signal_syncs.end(),
+ [](butil::intrusive_ptr<SignalSync>& sync) {
+ return sync->ref_count() == 0;
+ });
+ _inuse_signal_syncs.erase(iter, _inuse_signal_syncs.end());
// Each signal trace has an independent SignalSync to
// prevent the previous SignalHandler from affecting the new SignalTrace.
- butil::intrusive_ptr<SignalSync> signal_sync(new SignalSync());
+ Result result;
+ butil::intrusive_ptr<SignalSync> signal_sync(new SignalSync(&result));
if (!signal_sync->Init()) {
- return Result::MakeErrorResult("Fail to init SignalSync");
+ result.SetError("Fail to init SignalSync");
+ return result;
}
+
// Add reference for SignalHandler.
signal_sync->AddRefManually();
- union sigval value{};
+ sigval value{};
value.sival_ptr = signal_sync.get();
size_t sigqueue_try = 0;
while (sigqueue(tid, SIGURG, value) != 0) {
if (errno != EAGAIN || sigqueue_try++ >= 3) {
- return Result::MakeErrorResult("Fail to sigqueue: %s", berror());
+ // Remove reference for SignalHandler.
+ signal_sync->RemoveRefManually();
+ result.MakeErrorResult("Fail to sigqueue: %s", berror());
Review Comment:
`MakeErrorResult` is a static factory method, not an instance method. This
line will not compile. Use `result.SetError(...)` or return
`Result::MakeErrorResult(...)` instead.
##########
src/bthread/task_tracer.cpp:
##########
@@ -362,85 +355,23 @@ void TaskTracer::SignalHandler(int, siginfo_t* info,
void* context) {
ErrnoGuard guard;
butil::intrusive_ptr<SignalSync> signal_sync(
static_cast<SignalSync*>(info->si_value.sival_ptr));
- if (NULL == signal_sync) {
+ Result* result = signal_sync->result;
+ if (NULL == signal_sync || NULL == result) {
Review Comment:
This dereferences `signal_sync` before checking if it is null. Swap the
null-check and the dereference to avoid potential crashes if `signal_sync` is
null.
```suggestion
if (NULL == signal_sync) {
// The signal is not from Tracer, such as TaskControl, do nothing.
return;
}
Result* result = signal_sync->result;
if (NULL == result) {
```
--
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]
