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

fix: clean up source and add publisher status #1568

Closed
wants to merge 2 commits into from

Conversation

NodeBoy2
Copy link

@NodeBoy2 NodeBoy2 commented Jan 9, 2020

Open the code for cleaning SrsSource, refer to: #713
At the same time, add a variable to check if SrsSource is active, and handle the case where fetch_or_create() is not used (publish or consume). Solve the issues #1509 and #1553. Test results can be found at: #1566


TRANS_BY_GPT3

@NodeBoy2
Copy link
Author

NodeBoy2 commented Jan 14, 2020

#209 The problem of being unable to delete HLS segment files when deleting SrsSource. Since deleting HLS depends on SrsSource in the pool, the hls_dispose parameter will be ineffective, and the HLS files cannot be deleted at regular intervals or when the service restarts (the expiration time specified by hls_window is valid). It is necessary to consider a solution for deleting HLS segments in the future.

TRANS_BY_GPT3

@ossrs ossrs deleted a comment from codecov-io Jan 15, 2020
@winlinvip
Copy link
Member

winlinvip commented Jan 15, 2020

Points that need to be confirmed:

  • Whether the slicing protocols like HLS and DASH are affected, as HLS is under Source and the slicing number and SequenceNumber of HLS need to be continuous in the next streaming.
  • There will be complex reference relationships, and there was a crash after enabling cleaning before, which needs to be confirmed. srs occasional crash caused by cleaning up unused SrsSource mechanism #713 #714

TRANS_BY_GPT3

@winlinvip
Copy link
Member

winlinvip commented Jan 16, 2020

In #1509, two points were optimized. When there are 4k different streams (sources), the memory decreased from 125MB to 45MB, and there will no longer be two coroutines for each source constantly residing.

  • Resolve coroutine issue: link
  • Reduce source memory usage: link

These two modifications have no impact on stability.

TRANS_BY_GPT3

@NodeBoy2
Copy link
Author

NodeBoy2 commented Jan 17, 2020

After deleting SrsSource, there is a problem that causes a coredump when accessing http-flv. When establishing a push stream, if http-flv is enabled, the stream will be mounted on the http-server, and the handler will establish a reference to SrsSource. However, there is no mechanism to unmount the stream, so releasing it will cause a coredump. It is necessary to clarify the impact caused by SrsSource. There is an idea, whether it is possible to introduce shared_ptr and weak_ptr to manage SrsSource? --- Deleting SrsSource will not cause a coredump. SrsSource will set the stream as unavailable in on_unpublish. It is because SrsLiveStream detects the stream as unavailable, it will recreate SrsSource and mount it to the http-server. Since there is no push stream, closing the client will not trigger the on_unpublish operation, which leads to a coredump when accessing it again.

TRANS_BY_GPT3

@winlinvip
Copy link
Member

winlinvip commented Jan 19, 2020

There is another idea to support smooth exit for SRS, and I have roughly thought about it:

  1. Remove the exclusive lock on the PID file, close the API port, and allow a new SRS to start.
  2. Use REUSEPORT to start a new SRS, where both the old and new SRS are providing services using the same PID file.
  3. The old SRS will no longer accept new connections. It will exit after serving existing clients or after a certain period of time, such as 12 hours.

In this way, the old SRS can easily and safely release the created sources. The only problem is that when both the old and new SRS are providing services, the API is provided by the new SRS. Therefore, the number of clients in the system will not be accurate, as the old SRS will miss counting the clients it is serving.

Remark: If it is a source cluster, the stream will be on the old SRS, which will result in the inability to detect the stream. In such cases, it is necessary to forcefully disconnect the stream. The client needs to support retries in order to smoothly support this, and an Edge can be placed before the source cluster to support retries in this way.

TRANS_BY_GPT3

@winlinvip
Copy link
Member

winlinvip commented Jan 19, 2020

I have provided a detailed description of the plan in #1579. If you agree with this plan, I will close this PR.

TRANS_BY_GPT3

@NodeBoy2 NodeBoy2 closed this Jan 19, 2020
@NodeBoy2
Copy link
Author

NodeBoy2 commented Jan 19, 2020

I have described the plan in detail in #1579. If you agree with this plan, I will close this PR.

Okay, you can close this PR. Meanwhile, I will continue to refine the logic relationship related to SrsSource. 🙏🙏🙏.

TRANS_BY_GPT3

@winlinvip
Copy link
Member

winlinvip commented Jan 19, 2020

Thank you, you did a great job and also inspired me to find new solutions. If there are better solutions, we can continue discussing them. I believe there is no best solution, only more stable and simpler ones.

TRANS_BY_GPT3

@duiniuluantanqin
Copy link
Member

duiniuluantanqin commented Jun 5, 2020

I have described the plan in detail in #1579. If you agree with this plan, I will close this PR.

Okay, you can close this PR. Meanwhile, I will continue to refine the logic relationship of SrsSource. 🙏🙏🙏.

Hey, may I ask if you have continued to refine this issue recently?
Because I don't think hot upgrade is necessarily the best solution. It would be more in line with the requirements if we can clean up the source in a timely manner.
If you have already resolved this issue, I would also appreciate it if you could submit a PR.

TRANS_BY_GPT3

@NodeBoy2
Copy link
Author

NodeBoy2 commented Jun 5, 2020

I have described the plan in detail in #1579. If you agree with this plan, I will close this PR.

Okay, you can close this PR. Meanwhile, I will continue to refine the logic relationship of SrsSource. 🙏🙏🙏.

Hey, may I ask if you have continued to refine this issue recently?
Because I don't think hot upgrade is necessarily the best solution. It would be more in line with the requirements if we can clean up the source in a timely manner.
If you have already resolved this issue, I would appreciate it if you could submit a PR.

The reference relationships involved in SrsSource are quite complex, and there are many issues with removing the imports. It poses a high risk to the production environment, so we have not continued to follow up on the plan to delete SrsSource.

TRANS_BY_GPT3

@duiniuluantanqin
Copy link
Member

duiniuluantanqin commented Jun 8, 2020

I have described the plan in detail in #1579. If you agree with this plan, I will close this PR.

Okay, you can close this PR. Meanwhile, I will continue to refine the logic relationship of SrsSource. 🙏🙏🙏.

Hey, may I ask if you have continued to refine this issue recently?
Because I think hot upgrade may not be the optimal solution, it would be more in line with the requirements to clean up the source in a timely manner.
If this issue has already been resolved, I would still appreciate it if you could submit a PR.

The reference relationships involved in SrsSource are quite complex, and there are many issues with deleting the imports. It poses a high risk to the production environment, so we have not continued to follow up on the plan to delete SrsSource.

When I load tested with 1000 streams, each at 720P with a 1Mbps bitrate, the memory usage increased to 3GB. This would make it impractical for commercial use. The author mentioned that the memory usage for 4K streams was only 45MB, but I'm not sure how they tested it.

Lastly, I wanted to ask if you handled this issue using the smooth exit method mentioned by the author, or if there are other solutions available?

Thank you very much!

TRANS_BY_GPT3

@NodeBoy2
Copy link
Author

NodeBoy2 commented Jun 8, 2020

I have described the plan in detail in #1579. If you agree with this plan, I will close this PR.

Okay, you can close this PR. Meanwhile, I will continue to refine the logic relationship of SrsSource. 🙏🙏🙏.

Hey, may I ask if you have continued to refine this issue recently?
Because I think hot upgrade may not be the optimal solution, it would be more in line with the requirements to clean up the source in a timely manner.
If this issue has already been resolved, I would appreciate it if you could submit a PR.

The reference relationships involved in SrsSource are quite complex, and there are many issues with deleting the imports. It poses a high risk to the production environment, so we have not continued to follow up on the plan to delete SrsSource.

I stress-tested with 1000 streams, each streaming at 720P with a bitrate of 1Mbps, and the memory usage increased to 3GB. This would make it unsuitable for commercial use. The author mentioned that 4K streams only occupy 45MB of memory, but I'm not sure how they tested it.

Lastly, may I ask if you handled this issue using the smooth exit method mentioned by the author? Or do you have any other solutions?

Thank you very much!

Currently, I am using the method of releasing SrsSource. However, the known issue is that it affects HLS cleanup and HTTP-FLV. The server currently does not have HTTP-FLV enabled, and the cleanup of HLS fragments is handled by another service. Mr. Yang has a fix for this issue in this link: #1509. It involves deleting the two resident coroutines of SrsSource when stopping the stream, and also changing the default size of the fast vector from 8k to 8 bytes. Have you tried merging this fix and testing it?

TRANS_BY_GPT3

@duiniuluantanqin
Copy link
Member

duiniuluantanqin commented Jun 11, 2020

The method I am currently using is to release SrsSource. However, the known issue at the moment is that it will affect hls cleaning and http-flv. The server temporarily does not have http-flv enabled, and cleaning hls slices is done by another service. Mr. Yang has fixed this issue in #1509 by deleting the two resident coroutines of SrsSource when stopping the stream, and also changing the default size of fast vector from 8k to 8 bytes. Have you tried merging and testing it?

Changing the default size of fast vector did not solve the problem. May I ask if you have uploaded the code for cleaning SrsSource to GitHub? Is it this one: https://github.com/NodeBoy2/srs/tree/fix_cleansource3.0/trunk? If it is, I will refer to it and make the necessary modifications. Thank you.

TRANS_BY_GPT3

@winlinvip
Copy link
Member

Dup to #1509
Solution: #1579 (comment)

@winlinvip winlinvip added the TransByAI Translated by AI/GPT. label Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TransByAI Translated by AI/GPT.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants