Add a catchable soft time limit per feed update#97
Open
pklampros wants to merge 1 commit into
Open
Conversation
When a feed is too big it can take a long time to parse. This is not necessarily a problem, but there's currently a 30 second limit on the parsing (so that it doesn't get stuck forever). The problem with set_time_limit is that it's a non-catchable error, so if one of the feeds does trigger it then the rest of the feeds in the update iteration do not get updated and there's no way to recover from the error to continue. Because the failing feed is also not marked as completed it will be retried next time and any feeds after it will keep failing as well. This change adds a catchable soft time limit at 30s and increases the hard php uncatchable limit to 120. The soft limit is tested when fetching every single item in a feed so if we get past it we can gently exit the feed update with a timeout error allowing the other updates to continue. It's still possible to hit the hard limit if an item (not the whole feed) takes too long to parse, but this would require even deeper checking against the soft limit
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a feed is too big it can take a long time to parse. This is not necessarily a problem, but there's currently a 30 second limit on the parsing (so that it doesn't get stuck forever). The problem with set_time_limit is that it's a non-catchable error, so if one of the feeds does trigger it then the rest of the feeds in the update iteration do not get updated and there's no way to recover from the error to continue. Because the failing feed is also not marked as completed it will be retried next time and any feeds after it will keep failing as well.
This change adds a catchable soft time limit at 30s and increases the hard php uncatchable limit to 120. The soft limit is tested when fetching every single item in a feed so if we get past it we can gently exit the feed update with a timeout error allowing the other updates to continue. It's still possible to hit the hard limit if an item (not the whole feed) takes too long to parse, but this would require even deeper checking against the soft limit.
The feed in question was: https://feeds.simplecast.com/54nAGcIl
Note: It is possible to "catch" the
set_time_limiterror using aregister_shutdown_function, but that kills the whole update iteration. In that case we'd need to register the failure somewhere (in the db?) so that it's not tried again - otherwise we have the same problem: Feeds after the failed one do not get updated.Note 2: Tangentially related: It's not possible to escape this (impossibility to update the rest of the feeds) even if you delete the offending feed from the interface because it's not actually deleted from the database and will be checked for updates next time the update is triggered.
Disclaimer: I used claude code to identify the issue and write some starting code. Checked, expanded and manually tested. Totally understandable if that's a problem and you'd rather not merge.