Skip to content

Edge: Improve stability for state and fd closing. v5.0.214 v6.0.139 #4126

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.

Sign up for GitHub

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

Jump to bottom
Merged
winlinvip merged 3 commits into from
Jul 24, 2024
Merged

Edge: Improve stability for state and fd closing. v5.0.214 v6.0.139 #4126

winlinvip merged 3 commits into from
Jul 24, 2024
+25 −16

Conversation

@winlinvip
Copy link
Member

@winlinvip winlinvip commented Jul 23, 2024 •
edited
Loading

  1. Should always stop coroutine before close fd, see Edge pull, the system often encounters errors ret=1018 (Device or resource busy) and ret=1018 (No such file or directory). #511, When the SrsHttpApi object is destructed, the program encounters a coredump, caused by srs_close_stfd. #1784
  2. When edge forwarder coroutine quit, always set the error code.
  3. Do not unpublish if invalid state.

Co-authored-by: Jacob Su suzp1984@gmail.com

@winlinvip winlinvip marked this pull request as ready for review July 23, 2024 08:50
suzp1984
suzp1984 reviewed Jul 23, 2024
View reviewed changes
Copy link
Contributor

@suzp1984 suzp1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review send_error_code problem.

trunk/src/app/srs_app_edge.cpp Outdated
if ((err = do_cycle()) != srs_success) {
// If coroutine stopping, we should always set the quit error code.
err = do_cycle();
if (send_error_code == 0) {
Copy link
Contributor

@suzp1984 suzp1984 Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (send_error_code == 0) {
if ((err = do_cycle()) != srs_success) {
if (send_error_code == ERROR_SUCCESS) {
// If coroutine stopping, we should always set the quit error code.
send_error_code = srs_error_code(err);
}
return srs_error_wrap(err, "do cycle");
}

suggest another way.

About send_error_code, I believe there is a dead loop:

srs/trunk/src/app/srs_app_edge.cpp

Lines 864 to 884 in 8c034e5

while (true) {
if ((err = trd->pull()) != srs_success) {
return srs_error_wrap(err, "edge forward pull");
}
if (send_error_code != ERROR_SUCCESS) {
srs_usleep(SRS_EDGE_FORWARDER_TIMEOUT);
continue;
}
// read from client.
if (true) {
SrsCommonMessage* msg = NULL;
err = sdk->recv_message(&msg);
if (err != srs_success && srs_error_code(err) != ERROR_SOCKET_TIMEOUT) {
srs_error("edge push get server control message failed. err=%s", srs_error_desc(err).c_str());
send_error_code = srs_error_code(err);
srs_error_reset(err);
continue;
}

there are no place to set send_error_code = ERROR_SUCCESS in the loop, only set the value when sdk->recv_message(&msg).

Copy link
Member Author

@winlinvip winlinvip Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not dead loop, the publish owner will stop it when error.

@winlinvip winlinvip changed the title Edge: Improve stability for state and fd closing. Edge: Improve stability for state and fd closing. v5.0.214 v6.0.139 Jul 24, 2024
@winlinvip winlinvip added the RefinedByAI Refined by AI/GPT. label Jul 24, 2024
@winlinvip winlinvip merged commit f04e939 into ossrs:develop Jul 24, 2024
winlinvip added a commit that referenced this pull request Jul 24, 2024
@winlinvip @suzp1984
Edge: Improve stability for state and fd closing. v5.0.214 (#4126)
ad0d510 
1. Should always stop coroutine before close fd, see #511, #1784
2. When edge forwarder coroutine quit, always set the error code.
3. Do not unpublish if invalid state.

---------

Co-authored-by: Jacob Su <suzp1984@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@suzp1984 suzp1984 suzp1984 approved these changes

@duiniuluantanqin duiniuluantanqin Awaiting requested review from duiniuluantanqin

@xiaozhihong xiaozhihong Awaiting requested review from xiaozhihong

Assignees

No one assigned

Labels

RefinedByAI Refined by AI/GPT.

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@winlinvip @suzp1984