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 fallback server option #1572

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lewisdoesstuff
Copy link

As the main server seems to be having some issues, I've added an option to use a fallback server when the main one fails.

This defaults to "" (undefined) in the config, with users being able to set the URL below the standard URL option in options.

If you've got testingServer set, this is overridden, and the form is removed from the options.

How it works:

Upon requesting sponsors, if the main server fails, we call retryFetch to try again. As we keep note of how many times we retry, we use the fallback server on every even retry, so it flips back and forth.

The delay between retries has also been adjusted. 404 is the same, but if we've failed 2 or less times, we retry instantly, before going back to the previous delay setting.

As most users are only concerned about fetching sponsors, we don't use the fallback server for requests regarding submitting data.

We also only use the fallback server on asyncRequestToServer() calls, as nothing using the synchronous method would benefit.

As mentioned in issues #1570 and #1562

  • I agree to license my contribution under LGPL-3.0 or my contribution is from another project with a license compatible with LGPL-3.0

@mini-bomba
Copy link
Contributor

mini-bomba commented Nov 4, 2022

There are some caveats with using mirrors that need to be kept in mind and accounted for.

  • Currently, most custom-software mirrors only implement the bare minimum - just the GET /skipSegments/ endpoint.
  • Mirrors will not accept any submissions.
    • Also any submission action will leak the privateID to a mirror/fallback server.

From what I can see in the diff, your changes probably sometimes forward more than just GET /api/skipSegments (most notably the POST /api/skipSegments endpoint, which is the submission endpoint mirrors currently will not accept)

Also a question to think about: Should the fact that a fallback server is being used be indicated somewhere?

@ajayyy
Copy link
Owner

ajayyy commented Nov 4, 2022

I personally think it should be skip segments only

@lewisdoesstuff
Copy link
Author

I definitely agree, the fallback should only be used as a mirror to pull from, other requests should still use the main server.

Let me make some changes to only have it on GET /api/skipSegments.

@mini-bomba - any ideas on what would work best for this? I wouldn't want something as intrusive as a browser alert every time it fetches from the fb. Potentially an alert when choosing to upload segments if we previously used the fallback server, or some static text in the dialog aside the video player? (I'm not sure what you'd call it).

@lewisdoesstuff
Copy link
Author

Well that broke my commit :/

Set fallbackServer to false on the POST /api/skipSegments endpoint
I'll have a think about implementing some kind of notification/status of which server's being used, as well as squash that commit history :)

@lewisdoesstuff lewisdoesstuff changed the base branch from master to april-fools-2022-firefox November 4, 2022 22:47
@lewisdoesstuff lewisdoesstuff changed the base branch from april-fools-2022-firefox to master November 4, 2022 22:48
@ajayyy
Copy link
Owner

ajayyy commented Nov 4, 2022

Probably makes most sense to have that in the popup. Instead of "Segments found on this video" say "Segments found on this video from the backup server"

As the main server seems to be having some issues, I've added an option to use a fallback server when the main one fails.

This defaults to "" (undefined) in the config, with users being able to set the URL below the standard URL option in options.

If you've got testingServer set, this is overridden, and the form is removed from the options.

How it works:

Upon requesting sponsors, if the main server fails, we call retryFetch to try again. As we keep note of how many times we retry, we use the fallback server on every even retry, so it flips back and forth.

The delay between retries has also been adjusted. 404 is the same, but if we've failed 2 or less times, we retry instantly, before going back to the previous delay setting.

As most users are only concerned about fetching sponsors, we don't use the fallback server for requests regarding submitting data.

We also only use the fallback server on asyncRequestToServer() calls, as nothing using the synchronous method would benefit.
@lewisdoesstuff
Copy link
Author

May have accidentally reverted to the commit before and force pushed :/

Readded these, squashed commits.

Instead of "Segments found on this video" say "Segments found on this video from the backup server"

This makes the most sense to me - Need some sleep but I'll get that done tomorrow hopefully.

@lewisdoesstuff lewisdoesstuff reopened this Nov 4, 2022
@goodness-from-me
Copy link

Maybe set some default fallback server? I don't think that average SponsorBlock user would search for fallback server by himself.

@mini-bomba
Copy link
Contributor

mini-bomba commented Nov 5, 2022

Maybe set some default fallback server? I don't think that average SponsorBlock user would search for fallback server by himself.

Could perhaps have a list of mirrors sent by the server, have the extension remember it for a few days and pick a random server from the list every time it updates the list, as long as the user didn't modify it.
Though maybe a select list would be better for this, with "Pick automatically", "Don't use a fallback server" and "Custom server" options....
Though what I've just thought of could have issues on firefox, where you have to ask for permission to communicate with another server...

But anyways, we probably should first implement the basic feature, then add further enhancements.

@ajayyy
Copy link
Owner

ajayyy commented Nov 5, 2022

No default fallback server for now

@lewisdoesstuff
Copy link
Author

Just finishing up implementing this, but noticed some strange behaviour - not sure if anyone more familiar may be able to shed some insight?

The fallback switch works fine when loading a video without the main server reachable, but after switching videos a few times (in the recommended pane), it seems to stop working.
upon further testing, it seems to fail on a new video as well, sometimes
The request to get the segments seems to be passed to utils.asyncRequestToServer and then to utils.asyncRequestToCustomServer with no issue, but it never gets a response from the listener in background.ts, nor the sendRequestToCustomServer in there.

It looks like it'll occasionally throw

Unchecked runtime.lastError: A listener indicated an asynchronous response by returning true, but the message channel closed before a response was received

at some point, but this happens on a successful retry request as well.

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

Successfully merging this pull request may close these issues.

4 participants