Skip to content

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

Merged
winlinvip merged 3 commits intoossrs:developfrom
winlinvip:bugfix/close_fd
Jul 24, 2024
Merged

Edge: Improve stability for state and fd closing. v5.0.214 v6.0.139#4126
winlinvip merged 3 commits intoossrs:developfrom
winlinvip:bugfix/close_fd

Conversation

@winlinvip
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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