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

403 errors are ignored #762

Closed
Ross-cz opened this issue Apr 26, 2017 · 9 comments
Closed

403 errors are ignored #762

Ross-cz opened this issue Apr 26, 2017 · 9 comments
Assignees
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@Ross-cz
Copy link
Contributor

Ross-cz commented Apr 26, 2017

  • What version of Shaka Player are you using?
    HEAD
  • Are you using the demo app or your own custom app?
    b
    • If custom app, can you reproduce the issue using our demo app?
      n
  • What did you do?
  1. setup proxy
  2. play content
  3. during play, change proxy to return 403 on any response
  • What did you expect to happen?
    There should be no download retries and severity non-recoverable.

  • What actually happened?
    Retries occurred and severity was overriden to recoverable inside method handleNetworkError_.

@joeyparrish
Copy link
Member

This should have been fixed in #620 such that there were no retries.

I'm fairly sure the severity should be "recoverable", though. In the case of a license request failure, we don't know it's fatal. If there were multiple requests that covered the same keys (which can happen with some content), the failure of an initial license request would not be fatal. In the case of a failed renewal, a later renewal could still extend the license before the keys expire. Because of these things, I'm not sure there's any way for us to confidently label such a failure as "fatal".

@joeyparrish joeyparrish added this to the v2.2.0 milestone Apr 26, 2017
@joeyparrish joeyparrish added the type: bug Something isn't working correctly label Apr 26, 2017
@joeyparrish
Copy link
Member

We will look into the retry issue. Thanks for the report!

@Ross-cz
Copy link
Contributor Author

Ross-cz commented Apr 27, 2017

Hi Joey,

thank you for detail logic explanation. I agree with you.

I just want to add that my issue was with segment request (m4s). :-) There were some unit tests, but it seems the error leaks from HttpPlugin to other layer untested (StreamingEngine - handleNetworkError_).

bhh1988 pushed a commit to bhh1988/shaka-player that referenced this issue Jun 2, 2017
Do not infinitely retry on top of the retry-policy already defined by
RetryParameters, especially for VOD. For live video the previous retry
logic is still useful to be robust against clock-sync or asset
availability issues. This commit adds a boolean configuration parameter,
infiniteRetriesForLiveStreams, to allow the user to turn this behavior
off even for live video.

Fixes shaka-project#830 and shaka-project#762
bhh1988 pushed a commit to bhh1988/shaka-player that referenced this issue Jun 2, 2017
Do not infinitely retry on top of the retry-policy already defined by
RetryParameters, especially for VOD. For live video the previous retry
logic is still useful to be robust against clock-sync or asset
availability issues. This commit adds a boolean configuration parameter,
infiniteRetriesForLiveStreams, to allow the user to turn this behavior
off even for live video.

Fixes shaka-project#830 and shaka-project#762
joeyparrish pushed a commit that referenced this issue Jun 5, 2017
Do not infinitely retry on top of the retry-policy already defined by
RetryParameters, especially for VOD. For live video the previous retry
logic is still useful to be robust against clock-sync or asset
availability issues. This commit adds a boolean configuration parameter,
infiniteRetriesForLiveStreams, to allow the user to turn this behavior
off even for live video.

Fixes #830 and #762
joeyparrish pushed a commit that referenced this issue Jun 5, 2017
Do not infinitely retry on top of the retry-policy already defined by
RetryParameters, especially for VOD. For live video the previous retry
logic is still useful to be robust against clock-sync or asset
availability issues. This commit adds a boolean configuration parameter,
infiniteRetriesForLiveStreams, to allow the user to turn this behavior
off even for live video.

Fixes #830 and #762
@joeyparrish
Copy link
Member

Cherry-picked to v2.1.3.

@Ross-cz
Copy link
Contributor Author

Ross-cz commented Jun 6, 2017

Hi, last change is too generic and doesn't support real world scenarios, when you want to have robust solution:

  1. service can return 404, when it is being deployed using simple strategy, e.g. no failover or backup secondary service. I would like to have retry support for this.
  2. 403 is just forbidden and 401 means challenge required - both are different and specifc. 401 means stupid configuration or missing cookie. 403 means stupid configuration or provided things expired.
  3. regres VOD timeout and http error is critical too without retry.
  4. handleNetworkError_ was used even for subtitles, now it is not needed anymore.

As for the infinity - this works in case of playing buffered data, but after that can you show me user, who is looking 2 mins into rotating wheel? We are fighting with every 10ms latency.. There should be really something more helpfull on the API side - more real world.
I would like to have ability to retry based on my own logic even critical network errors. This would cover all scenarios:

  • stupid backend deployments especially 3rd party
  • let user control, e.g. if he just closed his laptop and left video paused on some browser tab..
    I will be able to expand logic like this:
  • for stupid servers I want to add user ability to report / retry using button.
  • let user know, his session expired, so he needs to relogin / hit play again / buy again, because rent expired, etc.

Or if you are lazy, reload whole player at that position.. This doesn't help to server stats..

@TheModMaker
Copy link
Contributor

We allow specifying retry parameters in a RetryParameters[1] object. You can specify this in the player configurations .manifest.retryParameters, .drm.retryParameters, and .streaming.retryParameters. This allows you to set the retry delay and the number of times to retry. This is separate from the given change. So if your server gives a 404, we will retry using that configuration. For example:

player.configure({
  streaming: {
    retryParameters: {
      maxAttempts: 20
    }
  }
});

Also, we still raise the error event when we handle network errors. So if we get a 401 error, you can listen to the error event and signal to the user that they need to reauthenticate.

We don't allow restarting retries after we have stopped. With the given change, we no longer retry forever for VOD, so a 403 error for VOD will stop playback. We could add a configuration to allow for infinite retries for VOD or add a call to start retrying again.

@TheModMaker
Copy link
Contributor

@Ross-cz What kind of API are you interested in? Would having an infiniteRetriesForVodStreams configuration be enough, or do you want something like a player.retryStreamLoading() call?

@Ross-cz
Copy link
Contributor Author

Ross-cz commented Jun 28, 2017

Hi Jacob, I would prefer method player.retryStreamLoading(). This gives me freedom for UX logic implementation.
Thank you!

@joeyparrish
Copy link
Member

@Ross-cz, I'm closing this issue and merging it into #960, in which I've proposed a more general solution. Please provide feedback there. Thanks!

@shaka-project shaka-project locked and limited conversation to collaborators Mar 22, 2018
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

No branches or pull requests

4 participants