Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOURCE: Possible race condition in SrsSource publish/unpublish #742

Closed
winlinvip opened this issue Jan 16, 2017 · 7 comments
Closed

SOURCE: Possible race condition in SrsSource publish/unpublish #742

winlinvip opened this issue Jan 16, 2017 · 7 comments
Assignees
Labels
Bug It might be a bug. TransByAI Translated by AI/GPT.
Milestone

Comments

@winlinvip
Copy link
Member

winlinvip commented Jan 16, 2017

The publish and unpublish operations of SrsSource are not atomic, so when the Source becomes more complex, such as when there are thread switches, it may lead to race conditions.

TRANS_BY_GPT3

@winlinvip winlinvip added the Bug It might be a bug. label Jan 16, 2017
@winlinvip winlinvip added this to the srs 3.0 release milestone Jan 16, 2017
@winlinvip
Copy link
Member Author

winlinvip commented Jan 16, 2017

There is another consistency issue where some objects are expressed through construction and destruction, which means they do not support the restart method. These objects are relatively simple and do not require consideration of initial state or resetting of state. Examples of such objects are SrsProtocol and SrsStSocket. Objects that are constructed with a pointer are mostly not reusable.

Considering the possibility of reconnection, functions like restart or connect-close have been introduced for objects like SrsTcpClient and SrsSimpleRtmpClient. Is it necessary for these objects to be designed with the restart method? Is it necessary for them to have residual state, such as remembering the host and port for automatic reconnection? It seems that they do not.

Therefore, these APIs can be simplified to non-reusable objects. Only objects like SrsThread may require the restart method. Some resource management, such as file descriptors (fd), do not need to be reused after being closed, unless there is a very high frequency of restart, such as implementing automatic reconnection to a TCP server. However, this type of restart is closely related to the business logic.

Therefore, most objects can be considered not supporting restart. Although they have connect and close functions, these functions are only provided for convenience, where connect is called when needed after creating the object, and close does not necessarily need to support reconnection.

TRANS_BY_GPT3

@winlinvip
Copy link
Member Author

winlinvip commented Jan 17, 2017

timeout is sometimes in milliseconds (ms) and sometimes in microseconds (us), but currently it seems more appropriate to use ms for all cases.
In addition to directly converting ST to US, NO TIMEOUT is represented by -1, so special macros are defined for that.
Apart from directly converting ST to US, the system clock is also in US, for example, when obtaining the current time.
All applications should be converted to ms.

TRANS_BY_GPT3

@winlinvip
Copy link
Member Author

winlinvip commented Jan 19, 2017

SrsSource has many functions, but in reality, only the origin server is effective, such as DVR, HLS, Forward, and Transcode. Although the edge server may also trigger these functions, it is actually meaningless. Therefore, these functions of the origin server can be placed on an OriginHub, just like a hub where DVR, HLS, Forward, and Transcode are all connected.

For example, the configuration vhost.play.queue_length should conceptually only be applied to the play or downstream edge, which means it should be related to the connections involved in play. Currently, it is being applied to the upstream edge and forward as well, which is actually incorrect. It should be configured in their respective configuration fields, such as vhost.forward.queue_length.

In addition, the SH and metadata of the cache in the Source can be placed in an aggregated data object. It would be better to have an object to manage these caches instead of scattering them into separate objects.

TRANS_BY_GPT3

winlinvip added a commit that referenced this issue Jan 22, 2017
@winlinvip
Copy link
Member Author

winlinvip commented Jan 23, 2017

There is another type of problem, which is set_cid and get_cid. In fact, the cid of a thread should be given a chance to be specified at creation. For example, the recv thread in the rtmp connection must have the same id as the service cycle thread of the connection, both representing this connection. This means that cid is more of a logical unit for business, rather than the id of a coroutine. A business unit may involve multiple coroutines, but should only have one id.

TRANS_BY_GPT3

@winlinvip
Copy link
Member Author

winlinvip commented Jan 23, 2017

In addition, HTTP is the core function of SRS, so SRS3 must enable HTTP. The related macro definitions do not need to be checked, they must be set to TRUE. For example, the macro definition SRS_AUTO_HTTP_CALLBACK does not need to be checked.

The options SRS_HTTP_CALLBACK, SRS_HTTP_SERVER, and SRS_HTTP_API can no longer be disabled. If they are disabled, a warning will be given, but the HTTP functionality will still be enabled.

--without-http-api
--without-http-server
--without-http-callback

TRANS_BY_GPT3

winlinvip added a commit that referenced this issue Jan 30, 2017
@winlinvip winlinvip changed the title SrsSource publish/unpublish可能出现竞争条件 SOURCE: SrsSource publish/unpublish可能出现竞争条件 Mar 5, 2017
@winlinvip
Copy link
Member Author

winlinvip commented Mar 5, 2017

Keep the constructor simple, such as an empty constructor, just to create objects. Refer to 308c6fe and #786. SrsTcpClient uses SrsStSocket, and the constructor of SrsStSocket does not simply require the FD to be passed in. This FD can only be obtained after TcpClient connects, so TcpClient can only construct the StSocket object after connecting, instead of being able to construct it in the constructor. TcpClient provides the interface get_recv_bytes, which can be called after construction (calling the interface of StSocket returns 0), but if StSocket is NULL after construction, it will crash. This violates the integrity of the concept because the non-simple constructor causes a conflict in the interface concept.

TRANS_BY_GPT3

@winlinvip
Copy link
Member Author

Fixed in #1230

@winlinvip winlinvip self-assigned this Sep 18, 2021
@winlinvip winlinvip changed the title SOURCE: SrsSource publish/unpublish可能出现竞争条件 SOURCE: Possible race condition in SrsSource publish/unpublish Jul 28, 2023
@winlinvip winlinvip added the TransByAI Translated by AI/GPT. label Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug It might be a bug. TransByAI Translated by AI/GPT.
Projects
None yet
Development

No branches or pull requests

1 participant