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

[WIP] core: Add option to disable errors in feeds #1071

Merged
merged 1 commit into from
Oct 31, 2019
Merged

[WIP] core: Add option to disable errors in feeds #1071

merged 1 commit into from
Oct 31, 2019

Conversation

logmanoriginal
Copy link
Member

Bridge errors are currently included as part of the feed to
notify users about erroneous bridges (before that, bridges
silently failed).

This solution, however, can produce a high load of error
messages if servers are down (see #994 for more details).

Admins may also not want to include error messages in feeds
in order to keep those kind of problems away from users or
simply to silently fail by choice.

This commit adds a new configuration section "error" with
one option "in_feed" which can be set to "true" to enable
the current behaviour (this is the default option) or to
"false" to disable it.

References #1066

@logmanoriginal logmanoriginal self-assigned this Mar 22, 2019
@somini
Copy link
Contributor

somini commented Mar 24, 2019

In that case, does the error get silently dropped? Can't the bridge requests request some kind of error?

https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#5xx_Server_errors

503 looks good to me.

@logmanoriginal
Copy link
Member Author

@somini You mean to return errors this way, right?

header('Content-Type: text/html', true, $e->getCode());

That was actually the original code before returning errors in the feed. I suppose if the option is disabled we can revert to the original behavior.

@somini
Copy link
Contributor

somini commented Apr 30, 2019

I think that's what I mean. Not just keeping the feed clean of "Bridge returned error" false items, but exposing the error as a HTTP status code for the Bridge request.

I agree that this being a breaking change can be disabled by default.

@logmanoriginal
Copy link
Member Author

I changed the implementation to allow for more options. Three are available right now:

  • feed => Errors are included in the feed (default)
  • http => Errors are returned as HTTP status code and some HTML content
  • none => Error reporting disabled

@somini Does that work for you?

@somini
Copy link
Contributor

somini commented Jun 19, 2019

LGTM, I have configured my personal instance with the "http" configuration, let's see what happens in the next few days.

Bridge errors are currently included as part of the feed to
notify users about erroneous bridges (before that, bridges
silently failed).

This solution, however, can produce a high load of error
messages if servers are down (see #994 for more details).

Admins may also not want to include error messages in feeds
in order to keep those kind of problems away from users or
simply to silently fail by choice.

This commit adds a new configuration section "error" with
one option "output" which can be set to following values:

"feed": To inclue error messages in the feed (default)
"http": To return a HTTP header for each error
"none": To disable error reporting

Note that errors are always logged to 'error.log' independend
of the settings above.

References #1066
@logmanoriginal
Copy link
Member Author

LGTM, I have configured my personal instance with the "http" configuration, let's see what happens in the next few days.

Any news on that?

@somini
Copy link
Contributor

somini commented Jul 7, 2019

This solves the problem without any side effects.

I still get rate-limited sometimes, but since RSS Bridge sends a HTTP error, the feed reader tries again after some time.
I double checked, and the feed reader is detecting some 503 errors, but it doesn't even expose this problems in the interface, it tries again after some time and the rate limits are gone by then.

This is completely transparent. No more erroneous "Bridge returned error" feed items.

This could even be the default, seems semantically more meaningful, but it would be a breaking change. It should be OK mentioning this on the default config file and on the release notes.

@somini
Copy link
Contributor

somini commented Jul 27, 2019

I'm still running this on http mode and it's still OK, but I think a special case should be made for HTTP error 302 or 301. Those are the redirects, that should be mapped to some 5xx error instead, otherwise the feed reader will complain about:

302 response missing Location header

508 Loop Detected looks good.

This happened on the Twitter bridge before 2bb9480, it sends you in a redirect loop and those 302 errors are erroneously passed to the outside.

@somini
Copy link
Contributor

somini commented Aug 22, 2019

Any chance of merging this? I have been using it for months with no issues.

@somini
Copy link
Contributor

somini commented Sep 21, 2019

a special case should be made for HTTP error 302 or 301. Those are the redirects, that should be mapped to some 5xx error instead

508 Loop Detected looks good.

Implemented this on somini@eb21d6f, will run it on my instance to check for issues.

@somini
Copy link
Contributor

somini commented Oct 30, 2019

Friendly ping on this issue. I'm running this without issues since June.

Be advised about somini@eb21d6f, to avoid infinite loops. It's a small change.

@logmanoriginal
Copy link
Member Author

Thanks for the reminder, I'll merge this and #1179 now.

@logmanoriginal logmanoriginal merged commit e8536ac into RSS-Bridge:master Oct 31, 2019
@logmanoriginal logmanoriginal deleted the OptionalErrorInFeed branch October 31, 2019 18:36
@somini
Copy link
Contributor

somini commented Nov 1, 2019

@logmanoriginal I'm sorry, it seems this particular commit was overlooked:

somini@eb21d6f

This is treating the redirect error codes as special cases and send a different error. Otherwise, this might leads to issues.

@logmanoriginal
Copy link
Member Author

Sorry, totally forgot about that. Would you mind opening a PR for it?

somini added a commit to somini/rss-bridge that referenced this pull request Nov 10, 2019
This might lead to redirect loops.

See
RSS-Bridge#1071 (comment)

Cherry-picked from eb21d6f.
@somini
Copy link
Contributor

somini commented Nov 10, 2019

Sorry, totally forgot about that. Would you mind opening a PR for it?

Not sure why I did not do that in the first place. It's on #1359

logmanoriginal pushed a commit that referenced this pull request Dec 1, 2019
This might lead to redirect loops.

See
#1071 (comment)

Cherry-picked from eb21d6f.
infominer33 pushed a commit to web-work-tools/rss-bridge that referenced this pull request Apr 17, 2020
…#1071)

Bridge errors are currently included as part of the feed to
notify users about erroneous bridges (before that, bridges
silently failed).

This solution, however, can produce a high load of error
messages if servers are down (see RSS-Bridge#994 for more details).

Admins may also not want to include error messages in feeds
in order to keep those kind of problems away from users or
simply to silently fail by choice.

This commit adds a new configuration section "error" with
one option "output" which can be set to following values:

"feed": To include error messages in the feed (default)
"http": To return a HTTP header for each error
"none": To disable error reporting

Note that errors are always logged to 'error.log' independent
of the settings above.

Closes RSS-Bridge#1066
infominer33 pushed a commit to web-work-tools/rss-bridge that referenced this pull request Apr 17, 2020
This might lead to redirect loops.

See
RSS-Bridge#1071 (comment)

Cherry-picked from eb21d6f.
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.

None yet

2 participants