wasphin commented on code in PR #3543:
URL: https://github.com/apache/brpc/pull/3543#discussion_r4059569855
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -269,12 +269,19 @@ 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.
+ // The guard structurally enforces the handoff: it drops the reference
+ // on any early return and is detached to the sender once it starts.
+ butil::intrusive_ptr<PlayingDummyStream> sender_ref(this);
int rc = bthread_start_background(&_play_thread, nullptr,
RunSendData, this);
if (rc) {
status->set_error(rc, "Fail to create thread");
return;
}
+ // The sender bthread now owns the reference held by the guard.
+ sender_ref.detach();
Review Comment:
There is still a race in the ownership handoff here.
`sender_ref` acquires a reference before `bthread_start_background()`, but
that same reference is adopted by `RunSendData()` with `add_ref=false`. The new
bthread may start, finish, and release the adopted reference before
`bthread_start_background()` returns and `sender_ref.detach()` is called. In
that case, both sides temporarily consider the same single reference as theirs,
so `detach()` does not provide a safe ownership transfer.
I think a simpler and more explicit approach is to reserve a reference
exclusively for the sender before starting the bthread, without keeping another
`intrusive_ptr` that owns the same reference:
```cpp
AddRefManually();
const int rc = bthread_start_background(
&_play_thread, nullptr, RunSendData, this);
if (rc != 0) {
RemoveRefManually();
// handle the error
}
```
Then let `RunSendData()` adopt that pre-acquired reference:
```cpp
static void* RunSendData(void* arg) {
butil::intrusive_ptr<PlayingDummyStream> self(
static_cast<PlayingDummyStream*>(arg), false);
self->SendData();
return nullptr;
}
```
This makes the ownership clear: the extra reference belongs to the sender as
soon as it is acquired. If bthread creation fails, the caller releases it; if
creation succeeds, only the sender releases it. It is therefore safe even if
the sender runs to completion before `bthread_start_background()` returns, and
no `detach()` handoff is needed.
--
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]
