SmartPtr: Use shared ptr in RTC TCP connection. v6.0.127#4083
SmartPtr: Use shared ptr in RTC TCP connection. v6.0.127#4083winlinvip merged 5 commits intoossrs:developfrom
Conversation
suzp1984
left a comment
There was a problem hiding this comment.
fix compiling error, when rtc is off.
./configure -rtc=off; make
f414449 to
f3fda03
Compare
suzp1984
left a comment
There was a problem hiding this comment.
wild pointer problem of SrsRtcTcpConn instance in conn_manager, the tcp connection resource manager.
|
|
||
| // 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.
| } | ||
|
|
||
| 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().
Fix issue #3784