SmartPtr: Use shared ptr in RTC TCP connection. v6.0.127 #4083
Conversation
winlinvip
force-pushed
the
feature/smart-ptr-rtc
branch
from
f414449
to
f3fda03
Compare
| @@ -1227,11 +1226,26 @@ srs_error_t SrsServer::do_on_tcp_client(ISrsListener* listener, srs_netfd_t& stf | |||
| } | |||
| } | |||
|
|
|||
| // For RTC TCP connection, use resource executor to manage the resource. | |||
| SrsRtcTcpConn* raw_conn = dynamic_cast<SrsRtcTcpConn*>(resource); | |||
| if (raw_conn) { | |||
There was a problem hiding this comment.
Be careful the raw_conn (SrsRtcTcpConn resource) and its conn (SrsSharedResource wrapper), the raw_conn will be add to conn_manager, the tcp connection resource manager of SrsServer, in line 1244, and its wrapper, a SrsSharedResource, will created below in line 1233, this wrapper didn't added to any resource manager, but it will be released by _srs_rtc_manager in SrsExecutorCoroutine's destructor. Then the raw_conn stored in conn_manager will became a wild pointer, the good news is that the conn_manager will never remove it, so the process will never crash, but it's still a problem, is it?
So, there are two problem:
- the
raw_connwill be added toconn_manager, which will never remove it; - the
raw_conninsideconn_managerwill became wild pointer once its wapper, theSrsSharedResourcereleased by_srs_rtc_managerinsideSrsExecutorCoroutine's desctructor.
There was a problem hiding this comment.
Nop, manager never adds raw_conn:
// For RTC TCP connection, use resource executor to manage the resource.
SrsRtcTcpConn* raw_conn = dynamic_cast<SrsRtcTcpConn*>(resource);
if (raw_conn) {
// ......
return err;
}Won't fix this.
| @@ -436,19 +436,19 @@ srs_error_t SrsRtcUdpNetwork::write(void* buf, size_t size, ssize_t* nwrite) | |||
| return sendonly_skt_->sendto(buf, size, SRS_UTIME_NO_TIMEOUT); | |||
| } | |||
|
|
|||
| SrsRtcTcpNetwork::SrsRtcTcpNetwork(SrsRtcConnection* conn, SrsEphemeralDelta* delta) | |||
| SrsRtcTcpNetwork::SrsRtcTcpNetwork(SrsRtcConnection* conn, SrsEphemeralDelta* delta) : owner_(new SrsRtcTcpConn()) | |||
There was a problem hiding this comment.
Current branch seems rebased on the #4084 now, which already supported constructor the SrsSharedResource instance from a NULL pointer, so I guess this initialization can be improved by:
| SrsRtcTcpNetwork::SrsRtcTcpNetwork(SrsRtcConnection* conn, SrsEphemeralDelta* delta) : owner_(new SrsRtcTcpConn()) | |
| SrsRtcTcpNetwork::SrsRtcTcpNetwork(SrsRtcConnection* conn, SrsEphemeralDelta* delta) : owner_(NULL) |
There was a problem hiding this comment.
No, reconsider this suggestion and let SrsSharedResource accept NULL pointer constructor, because
srs/trunk/src/app/srs_app_conn.hpp
Lines 163 to 165 in 7b9c52b
in this case, it's owner_->interrupt().
winlinvip
changed the title
Fix issue #3784