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

Added response to error code for bad http status #9167

Closed
wants to merge 2 commits into from
Closed

Added response to error code for bad http status #9167

wants to merge 2 commits into from

Conversation

JasonT962
Copy link

@JasonT962 JasonT962 commented Oct 21, 2022

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

A lot of people (including me) have been getting a network error 403 response code randomly when starting a new video. 403 response code is when the client is forbidden from accessing a URL. A common fix for this was to wipe your cached metadata in settings and then reloading the video.

This PR works by wiping your metadata cache and then reloading the video playback if the player runs into any bad http status error.

Fixes the following issue(s)

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

Clears metadata cache and reloads playback when player runs into bad http status error code
Copy link
Member

@AudricV AudricV left a comment

Choose a reason for hiding this comment

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

Hmm, that's not a good behavior in my opinion to clear the cache and retry each time this issue occurs.

For YouTube, this is an A/B test, they could roll out this change globally in UK or in the entire world, making your workaround useless.

This error affects other apps, even reVanced if I am not wrong, and must be fixed by extractor changes (and that's pretty easy in fact, by changing the primary InnerTube client used for playback (ANDROID) or removing it, unless a real fix with it is found).

However, I think we could retry to get stream info only one time when this type of error occurs, without clearing cache. The expiration time is managed by NewPipe's cache system for each service. This would solve 403 HTTP errors when stream URLs have expired (for instance on YouTube and SoundCloud).

@JasonT962
Copy link
Author

JasonT962 commented Oct 23, 2022

However, I think we could retry to get stream info only one time when this type of error occurs, without clearing cache. The expiration time is managed by NewPipe's cache system for each service. This would solve 403 HTTP errors when stream URLs have expired (for instance on YouTube and SoundCloud).

I don't mind changing it so it only retries to get stream info 1 time without clearing cache but I don't see how I would be able to do that without introducing a new private variable for Player.java to keep track of if it encounters the same error twice in a row. So, am I allowed to do that or no?

@JasonT962
Copy link
Author

JasonT962 commented Oct 23, 2022

I can also keep the original clear cache but it retries it up to x times before giving an error message? Like maybe 3 times?

When running into a bad http status error code, metadata cache will be wiped and playback will be reloaded up to 3 times before giving an error.
@JasonT962
Copy link
Author

JasonT962 commented Oct 23, 2022

So I've limited the amount of times the playback could be reloaded before it gave an error message. I've currently set the limit to 3 as with 1 attempt, the problem is a lot less common but still happens occasionally, with 2 attempts the problem was basically gone so I just went with 3 attempts to be safe.

If you still want to go with your original suggestion where it doesn't wipe cache and only reloads once, then all is needed to do is set the limit to 1 and remove the InfoCache.getInstance().clearCache(); line. However, I feel like it would be fine to keep it as this is supposed to be a more general response to the error code rather than a specific fix for the 403 response code.

@JasonT962 JasonT962 requested a review from AudricV October 24, 2022 07:44
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Would it be possible to clear the cache only of the currently playing video? That way we prevent the whole cache from being deleted (cache is there for a reason, to make things faster, so deleting it completely does not seem like a good idea).

Also, I think the badHttpStatusRetry variable should be reset to 0 whenever the currently playing stream changes. Otherwise, with the current implementation, only the first 3 videos with failures would benefit from retrying.

if (badHttpStatusRetry < 3) {
badHttpStatusRetry += 1;
isCatchableException = true;
// Clears metadata cache and then reloads playback
Copy link
Member

Choose a reason for hiding this comment

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

Add more comments here, with what you have in the PR description.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if you can only clear the cache of the currently playing video.

However, I have tested the badHttpStatusRetry variable thing and it does reset back to 0 automatically whenever the player changes. For example, if you watch a different video. I think it's probably because the Player.java class gets created every time you watch a new video.

@JasonT962 JasonT962 closed this Dec 8, 2022
@JasonT962 JasonT962 deleted the 403-HTTP-response-code-fix branch December 8, 2022 14:13
@opusforlife2
Copy link
Collaborator

@JasonT962 ?

@JasonT962
Copy link
Author

Sorry, I didn't think my workaround was necessary anymore, I also don't have the time to work on this right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants