Copilot commented on code in PR #3543:
URL: https://github.com/apache/brpc/pull/3543#discussion_r4013961475
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -290,15 +294,22 @@ class PlayingDummyStream : public brpc::RtmpServerStream {
LOG(INFO) << "OnStop of PlayingDummyStream=" << this;
if (_state.exchange(STATE_STOPPED) == STATE_PLAYING) {
bthread_stop(_play_thread);
- bthread_join(_play_thread, nullptr);
+ // A send failure can invoke this callback in the sender itself.
+ // Its own reference keeps the stream alive until SendData returns.
+ if (_play_thread != bthread_self()) {
+ bthread_join(_play_thread, nullptr);
+ }
}
Review Comment:
When `OnStop()` runs on the sender bthread, the current logic skips
`bthread_join()` but still calls `bthread_stop(_play_thread)` on itself.
Stopping the current bthread can be unsafe (depending on bthread semantics) and
may interfere with normal unwinding/cleanup. Consider moving
`bthread_stop(_play_thread)` inside the same-thread check (i.e., only stop/join
when `_play_thread != bthread_self()`), or otherwise document/handle the
self-stop case explicitly.
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -290,15 +294,22 @@ class PlayingDummyStream : public brpc::RtmpServerStream {
LOG(INFO) << "OnStop of PlayingDummyStream=" << this;
if (_state.exchange(STATE_STOPPED) == STATE_PLAYING) {
bthread_stop(_play_thread);
- bthread_join(_play_thread, nullptr);
+ // A send failure can invoke this callback in the sender itself.
+ // Its own reference keeps the stream alive until SendData returns.
+ if (_play_thread != bthread_self()) {
+ bthread_join(_play_thread, nullptr);
+ }
}
}
void SendData();
private:
static void* RunSendData(void* arg) {
- ((PlayingDummyStream*)arg)->SendData();
+ // Adopt the reference acquired before starting this bthread.
+ butil::intrusive_ptr<PlayingDummyStream> stream(
+ static_cast<PlayingDummyStream*>(arg), false);
+ stream->SendData();
Review Comment:
This change fixes a specific failure mode (send failure synchronously
triggering `OnStop()` on the sender bthread). To prevent regressions, add a
targeted regression test that deterministically triggers a send failure from
within `SendData()` and verifies the stream remains valid until the sender
exits (e.g., by asserting ordering of `OnStop` vs destructor, or by reproducing
the previously failing path under sanitizers).
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -269,9 +269,13 @@ class PlayingDummyStream : public brpc::RtmpServerStream {
<< " ms before responding play request";
bthread_usleep(_sleep_ms * 1000L);
}
+ // Keep the stream alive until the sender exits, even if a failed send
+ // synchronously runs OnStop() and releases the framework's references.
+ AddRefManually();
int rc = bthread_start_background(&_play_thread, nullptr,
RunSendData, this);
if (rc) {
+ RemoveRefManually();
status->set_error(rc, "Fail to create thread");
return;
}
Review Comment:
The correctness here depends on pairing `AddRefManually()` with an
'adopting' release in the sender thread. This pattern is easy to accidentally
break during future edits (e.g., changing the adopting behavior or adding new
early returns). Consider encapsulating this transfer in a small RAII helper
(e.g., a local guard that calls `RemoveRefManually()` unless explicitly
released to the sender), so the ownership handoff is enforced structurally
rather than by convention.
--
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]
