FeedsProcessor::clear() and ::expire() don't handle limit of 0 correctly [#2913403]

The comment above FeedsProcessor::getLimit() states that 0 means unlimited. On the sites I run, we set the limit to 0 because we perform all our feed processing in scheduled cron runs, so limiting the number of items that get processed in one run just increases the time it takes to process larger updates.

FeedsProcessor::clear() and ::expire() pass the return value from getLimit() directly to $select->range() as the second argument. When getLimit() returns 0, this results in a range from 0 to 0, which means no items get cleared or expired.

Comments

Comment #1

3 October 2017 at 00:20

j.matthew created an issue.

Comment #2

MegaChriz CreditAttribution: MegaChriz as a volunteer commented 3 October 2017 at 15:34

I see. Here are the relevant code lines.

From FeedsProcessor::clear():

$entity_ids = $select->range(0, $this->getLimit())->execute()->fetchCol();
$this->entityDeleteMultiple($entity_ids);

From FeedsProcessor::expire():

// Delete a batch of entities.
$entity_ids = $select->range(0, $this->getLimit())->execute()->fetchCol();
if ($entity_ids) {
  $this->entityDeleteMultiple($entity_ids);
  $state->deleted += count($entity_ids);
  $state->progress($state->total, $state->deleted);
}
else {
  $state->progress($state->total, $state->total);
}

If we simply skip the call to range() when getLimit() returns 0, PHP could potentially run out of memory if the list of entity ID's is very large. So maybe the code should be wrapped inside a while loop when the limit is 0? Suggestions or patches are welcome.

Comment #3

j.matthew CreditAttribution: j.matthew commented 4 October 2017 at 06:56

I think that's one of the factors you have to take into consideration before setting feeds_process_limit to 0, and certainly something you'd have to think about in general if you were processing feeds so large that just storing the entity IDs takes a significant amount of memory.

Comment #4

MegaChriz CreditAttribution: MegaChriz as a volunteer commented 4 October 2017 at 07:25

It's not just the entity ID's. The entity ID's are passed to FeedsProcessor::entityDeleteMultiple(). The node processor then passes this to node_delete_multiple(). And that function first loads all nodes for which ID's are passed. See https://api.drupal.org/api/drupal/modules%21node%21node.module/function/.... That could take a huge amount of memory.