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

Don't retry loads for certain error types/codes #2844

Closed
LachlanMcKee opened this issue May 18, 2017 · 4 comments
Closed

Don't retry loads for certain error types/codes #2844

LachlanMcKee opened this issue May 18, 2017 · 4 comments
Assignees

Comments

@LachlanMcKee
Copy link

LachlanMcKee commented May 18, 2017

Issue description

From what I understand, the HlsMediaSource retry mechanism is quite simple. It attempts to load the master playlist, as well as subsequent playlists, multiple times (based on a configurable value) so long as a ParserException is not encountered (as this is deemed a fatal exception).

I believe that it is reasonable that certain responses from a server should also be treated as fatal, or perhaps more importantly, developers should be able to customize what they consider a fatal error to be.

For example, the product I am currently working implements Geo-blocking. Our CDN returns a 403 when the user is outside the specified region. In this scenario, I would not expect the player to attempt to fetch the manifest again. In fact due to a third party library I am using, the fact that the player attempts to fetch the manifest a second time is causing unexpected bugs. The retry mechanism is extremely useful, and do not wish to disable it to solve the issue.

To maintain backwards compatibility, the HlsMediaSource could still take an integer value, and another constructor could be created to expose a more feature rich retry class.

Is there merit to this suggestion, or is there a good reason to avoid making this change?

Reproduction steps

  1. Load a HLS video from a source that implements geo-blocking and returns a 403 response as a result.
  2. Notice that the player will attempt to load the HLS video multiple times.

Link to test content

Unfortunately this depends on the country you live in, so I cannot provide a reliable HLS URL. However, since the player never manages to load the URL, any server returning a 403 would be sufficient.

Version of ExoPlayer being used

2.4.0

Device(s) and version(s) of Android being used

Affects all devices and versions since this currently the intended behavior.

A full bug report captured from the device

Not required as this is not a bug.

@ojw28 ojw28 changed the title HlsMediaSource - Master manifest retry policy customization Don't retry fetches for certain error codes May 22, 2017
@ojw28 ojw28 changed the title Don't retry fetches for certain error codes Don't retry loads for certain error types/codes May 22, 2017
@ojw28
Copy link
Contributor

ojw28 commented May 22, 2017

I don't think this is HLS specific. We should get smarter about not retrying non-recoverable error types/codes in general, and apply consistent rules for all loads. As well as non-recoverable HTTP error codes, there are also cases like FileNotFoundException for local playback, and so on, where we should fail immediately.

@LachlanMcKee
Copy link
Author

LachlanMcKee commented May 22, 2017

That's a very good point. It would be great to have this consistent across the modules.

I'm looking forward to seeing this!

@0c6a183d
Copy link

I agree this that "retries" should be disabled by default on some modules. My DefaultHttpDataSourceFactory (#4004) currently uses (connectTimeoutMillis: (4000 / 4) instead of 4000 for testing as it takes 4 times as long to finish its "re-tries" to a dead server. Be good if there was an option like connectRetryCount: or similar. Has there been any movement on this ticket in a year or is each module slowly being changed to support these options? It would be great if the DefaultHttpDataSourceFactory could have this option added to 2.7.2!

ojw28 pushed a commit that referenced this issue Jun 5, 2018
This allows injection of custom implementations and configuration of
DefaultHlsPlaylistTracker without modifying the HlsMediaSource interface.

Issue:#2844

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=198846607
ojw28 pushed a commit that referenced this issue Jun 25, 2018
This allows injection of custom implementations and configuration of
DefaultHlsPlaylistTracker without modifying the HlsMediaSource interface.

Issue:#2844

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=198846607
ojw28 pushed a commit that referenced this issue Jul 12, 2018
Issue:#2844
Issue:#3370
Issue:#2981

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=204149284
ojw28 pushed a commit that referenced this issue Jul 17, 2018
Issue:#2844
Issue:#2981

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=204718939
ojw28 pushed a commit that referenced this issue Aug 6, 2018
Issue:#2844
Issue:#3370
Issue:#2981

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=206927295
@AquilesCanta
Copy link
Contributor

Please have a look at LoadErrorHandlingPolicy and let us know if it suits your needs.

Thanks!

@google google locked and limited conversation to collaborators Dec 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants