Greetings.

In using the feedapi module with my own custom parser and processor, I've run into a bug when refreshing a feed manually (by clicking the 'Refresh' tab on the node page view of a feedapi enabled node).

Basically, there are too many items in my feed for the refreshing to finish within the default 30 second PHP timeout. The function in feedapi that is called to refresh items (_feedapi_invoke_refresh()) takes a special parameter to indicate whether the refresh is part of cron job. If it is, then the code takes special precautions to make sure that the feed will take no more than it's configured % share of the cron exeuction time.

However, there's no similar check if the call to this function is the result of something other than cron (in this case, a manual refresh of the feed).

This patch adds code that changes the _feedapi_invoke_refresh() function to apply a similar logic to manual refreshes. If a PHP timeout is 5 seconds away, the function breaks out of the loop similar to the way _feedapi_node_purge() (in feedapi_node.module) breaks out of its loop when it can't purge all items from a feed within the PHP timeout.

To achieve this, I changed the code to call feedapi_cron_time() for manual refreshes.

feedapi_cron_time() calculates the time remaining for a feed's refresh during a cron job. I changed the name of this function to feedapi_time_remaining() since this patch results in its use for manual refreshes and I also added a parameter for a boolean to indicate whether or not this function should be computing the time remaining for a feed refresh during a cron job. If not, it calculates the time remaining until there is only 5 seconds left before a timeout.

-Ankur

CommentFileSizeAuthor
#4 feedapi.module.diff.txt3.17 KBankur
feedapi.module.diff.txt3.04 KBankur

Comments

aron novak’s picture

Status: Needs review » Needs work

I like this, really makes sense to introduce this managed refreshing on manual refresh as well. At least it's without drawbacks.
Somehow it breaks the simpletest tests. Can you debug it why?

"The inaccessible feed URL was inserted in the database." - this fails somehow.

dixon_’s picture

I like the idea. However, I'd rather land my Batch API patch to fix this #545304: Use Batch API to improve refresh batching

The Batch API is how to do it in Drupal IMHO. The code base will be more straight forward etc. Somehow it feels a little bit hacky to check for timeouts all the time. But maybe that's just me :)

parrottvision’s picture

subscribe

ankur’s picture

Status: Needs work » Needs review
StatusFileSize
new3.17 KB

Here's an updated patch. @aaron in #1, I tried to run the tests from the simpletest module, and I couldn't replicate the

"The inaccessible feed URL was inserted in the database" failure.

Not sure what could be going on here. The code modifications don't interfere in any way with the saving of a feed URL. The only thing that happens is that function feedapi_cron_time() is replaced with function feedapi_time_remaining().

Perhaps it could be something in your dev environment. Not sure.

aron novak’s picture

Status: Needs review » Reviewed & tested by the community

ankur: you're right, these changes are okay (according to the tests also)

alex_b’s picture

Title: no mechanism for protecting against timeout during manual refresh » Protect against timeout during manual refresh
Status: Reviewed & tested by the community » Needs work

Looking good, haven't tested though. Needs update of feedapi_time_remaining()