Network stall detection #3227

avelad · 2021-03-15T11:13:05Z

Closes: #1582

TheModMaker · 2021-03-15T17:00:33Z

Joey's suggestion in that issue was if we went a certain time without getting more data, we'd stall. This cancels the timer after the first progress, so this is basically a connection timeout. If you want that, I'd suggest naming it connectionTimeout instead, or change to reset the timer on each progress to keep it a stall timeout.

Also, since the code is basically the same between the two handlers, I'd suggest moving to NetworkingEngine. This also allows app-provided handlers to get this too without having to implement it.

avelad · 2021-03-15T17:04:38Z

I am going to implement both suggestions. Thanks!

avelad · 2021-03-15T19:35:33Z

I have tried to move it to NetworkingEngine, but if I abort the operation from there, it had an unwanted recursion, I have not managed to fix it, so in the end I have left it in each plugin.

I have fixed the use of stallTimeout and added connectionTimeout.

@TheModMaker can you review again?

lib/net/http_fetch_plugin.js

avelad · 2021-03-17T07:03:23Z

This is really very beneficial, after testing internally for hours, we have seen that this actually makes buffering less times, so the user experience is better. Note: in our case 'streaming.retryParameters.maxAttempts' is 15.

avelad · 2021-03-18T20:28:00Z

@TheModMaker can you review again?

joeyparrish

I would also like @TheModMaker's opinion again on doing this generically in NetworkingEngine vs in each plugin. If the plugin produces progress events, I feel as if you could do it generically. If the plugin does not produce progress events, though, it may always trigger connection or stall timeouts. Could NetworkingEngine tell which plugins would generate those events? My instinct is "no", but I haven't analyzed it.

joeyparrish · 2021-03-18T20:51:38Z

docs/tutorials/network-and-buffering-config.md

@@ -15,6 +15,8 @@ identical:
 ```js
 retryParameters: {
  timeout: 0,       // timeout in ms, after which we abort; 0 means never
+  stallTimeout: 0,  // stall timeout in ms, after which we abort; 0 means never


Can you set these in the docs to their default values, please?

If I understand you correctly, you want the default value in the ShakaPlayer configuration to use 0 for both new configurations, is that correct?

No, sorry. I mean that the docs should show the default values. You gave non-zero defaults, and I agree with those defaults.

Sorry for taking time to respond, Friday was bank holiday in Spain. I'll fix it now.

lib/net/http_fetch_plugin.js

lib/net/http_xhr_plugin.js

joeyparrish · 2021-03-18T20:58:32Z

test/net/networking_engine_unit.js

@@ -90,6 +90,8 @@ describe('NetworkingEngine', /** @suppress {accessControls} */ () => {
        backoffFactor: 0,
        fuzzFactor: 0,
        timeout: 0,
+        stallTimeout: 0,


Would a new test case make sense?

I valued it, but the problem is that there is no timeout test to be able to compare :S

I understand. It's tough sometimes to figure out new test cases without a similar example to work from. Please file an issue to follow-up on this with tests, and we'll try to add some later. Don't worry about it for now. The code looks clear and short enough to me. (I hope I don't regret saying that some day! 😛)

Once merged I create the issue and link it here.

avelad · 2021-03-22T08:04:05Z

@joeyparrish I have moved the logic to NetworkingEngine. There is nothing that is exported to know if the plugin emits progress events or not, but since both xhr and fetch emit it, I think that right now it is not necessary to add this logic.

joeyparrish · 2021-03-22T16:58:22Z

There is nothing that is exported to know if the plugin emits progress events or not, but since both xhr and fetch emit it, I think that right now it is not necessary to add this logic.

That could still cause problems for custom plugins. For example, there are apps and other libraries which plug into the networking engine in order to load content over peer-to-peer connections. If those have not implemented progress events, we would see them as having stalled, correct?

Here's a nice compromise: We can keep the centralized code for stalls and timeouts, and add a flag to network plugins to declare that they do emit progress events. Then you can add this flag to the fetch & XHR plugins. This would allow us to have this feature without breaking existing plugins.

What do you think?

joeyparrish

Looks good apart from the need for a flag to avoid breaking old plugins

avelad · 2021-03-22T17:00:59Z

I think we can add a method in the plugins that tells us if they support progress events or not. Now in a few minutes I upload a commit with this change.

avelad · 2021-03-22T17:18:15Z

@joeyparrish I have already added what you have commented, can you review again?

joeyparrish

Thanks for taking care of this!

lib/net/networking_engine.js

lib/net/http_fetch_plugin.js

avelad · 2021-03-24T17:23:10Z

@joeyparrish Do you think you could have time today to approve and merge this PR?

avelad · 2021-03-30T17:39:35Z

@joeyparrish can you run the Shaka-bot?

michellezhuogg · 2021-03-30T17:44:04Z

@joeyparrish can you run the Shaka-bot?

Running that now :)

shaka-bot · 2021-03-30T18:00:22Z

All tests passed!

avelad · 2021-03-30T18:02:50Z

Thanks @michellezhuogg !

Network stall detection

4eb2225

Add connectionTimeout and fix usage of stallTimeout

ee01bab

caridley reviewed Mar 15, 2021

View reviewed changes

lib/net/http_fetch_plugin.js Outdated Show resolved Hide resolved

caridley reviewed Mar 15, 2021

View reviewed changes

lib/net/http_fetch_plugin.js Outdated Show resolved Hide resolved

Fix timers in fetch plugin

80a1107

Merge branch 'master' into network-stall-detection

7bc4a41

joeyparrish requested changes Mar 18, 2021

View reviewed changes

Alvaro Velad added 5 commits March 22, 2021 07:48

Merge branch 'master' into network-stall-detection

8812a78

Update tutorial

9ad5a3f

Remove useless timer stop

d7d6248

Move all logic to network engine

98022be

Add missing comment

4ad43bb

avelad requested a review from joeyparrish March 22, 2021 08:04

joeyparrish reviewed Mar 22, 2021

View reviewed changes

Check if the plugin supports progress events

235cc09

avelad requested a review from joeyparrish March 22, 2021 17:18

joeyparrish requested changes Mar 22, 2021

View reviewed changes

lib/net/networking_engine.js Outdated Show resolved Hide resolved

lib/net/http_fetch_plugin.js Outdated Show resolved Hide resolved

Simplify the code

c034b9c

avelad requested a review from joeyparrish March 22, 2021 17:51

joeyparrish merged commit 863e345 into shaka-project:master Mar 30, 2021

avelad deleted the network-stall-detection branch March 31, 2021 09:41

github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network stall detection #3227

Network stall detection #3227

avelad commented Mar 15, 2021

TheModMaker commented Mar 15, 2021

avelad commented Mar 15, 2021

avelad commented Mar 15, 2021

avelad commented Mar 17, 2021

avelad commented Mar 18, 2021

joeyparrish left a comment

joeyparrish Mar 18, 2021

avelad Mar 18, 2021

joeyparrish Mar 19, 2021

avelad Mar 22, 2021 •

edited

joeyparrish Mar 18, 2021

avelad Mar 18, 2021

joeyparrish Mar 19, 2021

avelad Mar 22, 2021

avelad commented Mar 22, 2021

joeyparrish commented Mar 22, 2021

joeyparrish left a comment

avelad commented Mar 22, 2021

avelad commented Mar 22, 2021

joeyparrish left a comment

avelad commented Mar 24, 2021

avelad commented Mar 30, 2021

michellezhuogg commented Mar 30, 2021

shaka-bot commented Mar 30, 2021

avelad commented Mar 30, 2021

Network stall detection #3227

Network stall detection #3227

Conversation

avelad commented Mar 15, 2021

TheModMaker commented Mar 15, 2021

avelad commented Mar 15, 2021

avelad commented Mar 15, 2021

avelad commented Mar 17, 2021

avelad commented Mar 18, 2021

joeyparrish left a comment

Choose a reason for hiding this comment

joeyparrish Mar 18, 2021

Choose a reason for hiding this comment

avelad Mar 18, 2021

Choose a reason for hiding this comment

joeyparrish Mar 19, 2021

Choose a reason for hiding this comment

avelad Mar 22, 2021 • edited

Choose a reason for hiding this comment

joeyparrish Mar 18, 2021

Choose a reason for hiding this comment

avelad Mar 18, 2021

Choose a reason for hiding this comment

joeyparrish Mar 19, 2021

Choose a reason for hiding this comment

avelad Mar 22, 2021

Choose a reason for hiding this comment

avelad commented Mar 22, 2021

joeyparrish commented Mar 22, 2021

joeyparrish left a comment

Choose a reason for hiding this comment

avelad commented Mar 22, 2021

avelad commented Mar 22, 2021

joeyparrish left a comment

Choose a reason for hiding this comment

avelad commented Mar 24, 2021

avelad commented Mar 30, 2021

michellezhuogg commented Mar 30, 2021

shaka-bot commented Mar 30, 2021

avelad commented Mar 30, 2021

avelad Mar 22, 2021 •

edited