fix: Exclude TIMEOUT errors when disabling streams #7369
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Incremental code coverage: 100.00% |
theodab
approved these changes
Sep 25, 2024
avelad
reviewed
Sep 25, 2024
The TIMEOUT error is what triggers the loading loop described in #7368. |
|
See my latest comments in the issue. |
joeyparrish
force-pushed
the
fix-vod-loop-1
branch
from
9b2bb07 to
9e090d2
Compare
joeyparrish
changed the title
JulianDomingo
approved these changes
Sep 26, 2024
joeyparrish
added a commit
that referenced
this pull request
Sep 26, 2024
In #7368, we get stuck in a loop loading forever. This regression was introduced in v4.4.0 and affects all v4.4, v4.5, v4.6, v4.7, and v4.8 releases, as well as v4.9.0-28, v4.9.2-caf1, v4.10.0-20, and v4.11.0-6. The loop is composed of these elements: 1. an error that triggers disabling a stream 2. an error that doesn't resolve itself over time 3. an error that is slow enough to trigger that the first streams get re-enabled 4. VOD content that doesn't change while we sit in the loop 5. enough streams to avoid exhausting them during the cycle Only `TIMEOUT` errors can trigger this bug AFAICT, so we should exclude those from the logic to disable streams. Note also that live streaming already retries indefinitely by default, and that normal ABR logic will change streams for us if we timeout due to a lack of bandwidth. Disabling streams on `TIMEOUT` was suggested initially in #4764, but was not a requirement of the OP. It was added out of caution in #4769, but not really vetted. Because it was not ever explicitly needed, excluding it is not a regression. Closes #7368 Backported to v4.9.2-caf Release-As: 4.9.2-caf2
joeyparrish
added a commit
that referenced
this pull request
Sep 26, 2024
In #7368, we get stuck in a loop loading forever. This regression was introduced in v4.4.0 and affects all v4.4, v4.5, v4.6, v4.7, and v4.8 releases, as well as v4.9.0-28, v4.9.2-caf1, v4.10.0-20, and v4.11.0-6. The loop is composed of these elements: 1. an error that triggers disabling a stream 2. an error that doesn't resolve itself over time 3. an error that is slow enough to trigger that the first streams get re-enabled 4. VOD content that doesn't change while we sit in the loop 5. enough streams to avoid exhausting them during the cycle Only `TIMEOUT` errors can trigger this bug AFAICT, so we should exclude those from the logic to disable streams. Note also that live streaming already retries indefinitely by default, and that normal ABR logic will change streams for us if we timeout due to a lack of bandwidth. Disabling streams on `TIMEOUT` was suggested initially in #4764, but was not a requirement of the OP. It was added out of caution in #4769, but not really vetted. Because it was not ever explicitly needed, excluding it is not a regression. Closes #7368
joeyparrish
added a commit
that referenced
this pull request
Sep 26, 2024
In #7368, we get stuck in a loop loading forever. This regression was introduced in v4.4.0 and affects all v4.4, v4.5, v4.6, v4.7, and v4.8 releases, as well as v4.9.0-28, v4.9.2-caf1, v4.10.0-20, and v4.11.0-6. The loop is composed of these elements: 1. an error that triggers disabling a stream 2. an error that doesn't resolve itself over time 3. an error that is slow enough to trigger that the first streams get re-enabled 4. VOD content that doesn't change while we sit in the loop 5. enough streams to avoid exhausting them during the cycle Only `TIMEOUT` errors can trigger this bug AFAICT, so we should exclude those from the logic to disable streams. Note also that live streaming already retries indefinitely by default, and that normal ABR logic will change streams for us if we timeout due to a lack of bandwidth. Disabling streams on `TIMEOUT` was suggested initially in #4764, but was not a requirement of the OP. It was added out of caution in #4769, but not really vetted. Because it was not ever explicitly needed, excluding it is not a regression. Closes #7368
joeyparrish
added a commit
that referenced
this pull request
Sep 26, 2024
In #7368, we get stuck in a loop loading forever. This regression was introduced in v4.4.0 and affects all v4.4, v4.5, v4.6, v4.7, and v4.8 releases, as well as v4.9.0-28, v4.9.2-caf1, v4.10.0-20, and v4.11.0-6. The loop is composed of these elements: 1. an error that triggers disabling a stream 2. an error that doesn't resolve itself over time 3. an error that is slow enough to trigger that the first streams get re-enabled 4. VOD content that doesn't change while we sit in the loop 5. enough streams to avoid exhausting them during the cycle Only `TIMEOUT` errors can trigger this bug AFAICT, so we should exclude those from the logic to disable streams. Note also that live streaming already retries indefinitely by default, and that normal ABR logic will change streams for us if we timeout due to a lack of bandwidth. Disabling streams on `TIMEOUT` was suggested initially in #4764, but was not a requirement of the OP. It was added out of caution in #4769, but not really vetted. Because it was not ever explicitly needed, excluding it is not a regression. Closes #7368
joeyparrish
pushed a commit
that referenced
this pull request
Sep 27, 2024
avelad
added
type: bug
avelad
pushed a commit
that referenced
this pull request
Oct 18, 2024
avelad
pushed a commit
that referenced
this pull request
Oct 18, 2024
joeyparrish
pushed a commit
that referenced
this pull request
Oct 18, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #7368, we get stuck in a loop loading forever. This regression was introduced in v4.4.0 and affects all v4.4, v4.5, v4.6, v4.7, and v4.8 releases, as well as v4.9.0-28, v4.9.2-caf1, v4.10.0-20, and v4.11.0-6.
The loop is composed of these elements:
Only
TIMEOUTerrors can trigger this bug AFAICT, so we should exclude those from the logic to disable streams. Note also that live streaming already retries indefinitely by default, and that normal ABR logic will change streams for us if we timeout due to a lack of bandwidth.Disabling streams on
TIMEOUTwas suggested initially in #4764, but was not a requirement of the OP. It was added out of caution in #4769, but not really vetted. Because it was not ever explicitly needed, excluding it is not a regression.Closes #7368