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

server/broadcast: don't retry segments after max attempts #2083

Merged
merged 3 commits into from
Nov 3, 2021

Conversation

jailuthra
Copy link
Contributor

@jailuthra jailuthra commented Nov 2, 2021

What does this pull request do? Explain your changes. (required)

Return 422 to push-api clients once the B has retried a segment MaxAttempts times.

Specific updates (required)

See commit history

How did you test each of these updates (required)

  1. Unit test to ensure 422 is returned when retried a segment MaxAttempts times
  2. Manual testing of Push API after returning an error for transcoding:
diff --git a/server/broadcast.go b/server/broadcast.go
index ff17a0d5..34c89e37 100644
--- a/server/broadcast.go
+++ b/server/broadcast.go
@@ -612,6 +612,9 @@ func transcodeSegment(cxn *rtmpConnection, seg *stream.HLSSegment, name string,
                        err = errors.New("empty response")
                }
                return nil, info, err
+       } else if sess != nil {
+               cxn.sessManager.completeSession(sess)
+               return nil, info, errors.New("Unknown error")
        }

        // download transcoded segments from the transcoder

Does this pull request close any open issues?

Fixes #2069

Checklist:

jailuthra added a commit that referenced this pull request Nov 2, 2021
@jailuthra jailuthra marked this pull request as ready for review November 2, 2021 09:44
Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -95,15 +89,40 @@ func TestPush_ShouldReturn422ForNonRetryable(t *testing.T) {

s.rtmpConnections["mani"] = cxn

req.Header.Set("Accept", "multipart/mixed")
// Should return 503 if got a retryable error with no sessions to use
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about whether there could be a case where B runs out of sessions before it hits the max # of attempts. Dumping some thoughts below just for visibility.

Based on

for len(attempts) < MaxAttempts {
I believe that if B runs out of sessions, it will still hit the max # of attempts - each of the remaining attempts will just result in an error caused by B being out of sessions. And since we check for a non-retryable error here and return before the no sessions available error check in the HTTP push handler we will always return a 422 in this scenario before getting to the code that triggers a 503.

@yondonfu yondonfu merged commit 637d659 into master Nov 3, 2021
@yondonfu yondonfu deleted the jai/max-retry-422 branch November 3, 2021 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return 422 to HTTP push client when max # of retries is hit
2 participants