This is a follow-up to #2820548: Fatal error when triggering Feeds via cron .

Problem/Motivation

Right now the entire feed object is serialized when queuing an import task for a feed. As a feed entity can be potentially big this could be a waste of database space in the queue. It can also cause PHP errors when certain information on the feed is outdated, for example if it contains references to no longer existing objects.
There's only one problem to this: besides the State objects on the feed, it is uncertain exactly which data on the feed is necessary during the various stages of import and therefore it is uncertain if reloading the feed entity from the database instead of storing it entirely and then unserializing it will cause issues.

About State objects
A feed import task is monitored by using State objects. A State object holds information about how many items were inserted, updated, deleted, skipped, etc. These State objects are only temporary apparent on a feed entity (on $feed->states) and saved on Drupal's key-value store after each queue task is done. When loading a feed queue task the State objects are taken from Drupal's key-value store again. In other words: State objects are always regenerated when unserializing a feed entity.

From #2820548-23: Fatal error when triggering Feeds via cron :

It could be that these State objects are everything what is stored temporary and in that case there is no issue with reducing the amount of data on the queue. But since there's a lot of code in Feeds, I cannot be sure of that and because of that we'd better have a test that covers the import process that needs multiple cron runs to complete.

Proposed resolution

To reduce the data in the queue, the solution would be to store only the feed ID on the queue task.

To ensure this doesn't cause issues for the import task, we need an automated test that covers the following case: running an import task that takes multiple cron runs to complete. Or, alternatively, ensure in any other way that an import process works using two (or more) separate PHP processes.
The reason we need two (or more) separate PHP processes for the import task is to rule out that the import is working because something happens to be still in the static cache.

Implementing the test happens in #3044983: Add test coverage for importing using multiple cron runs.

Remaining tasks

  1. #3044983: Add test coverage for importing using multiple cron runs DONE
  2. If possible, only store the feed ID on the queue task instead of the whole feed entity (the test results should reveal that this doesn't introduce issues). DONE
  3. Backwards compatibility: In \Drupal\feeds\Plugin\QueueWorker\FeedsRefresh, take into account that existing queue tasks may have a full feed entity serialized. DONE
  4. Backwards compatibility: In \Drupal\feeds\Plugin\QueueWorker\FeedsRefresh, take into account that existing queue tasks may have a full feed entity serialized that no longer exists. DONE
  5. Write a test that ensures that queue tasks with a full serialized feed entity can be processed and don't remain on the queue forever. DONE

API changes

To be determined.

Data model changes

When queuing a feed for import, only the feed ID will be stored on the queue task.

CommentFileSizeAuthor
#2 feeds-optimize-feeds-queue-2978490-2.patch4.15 KBmegachriz

Issue fork feeds-2978490

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

MegaChriz created an issue. See original summary.

megachriz’s picture

megachriz
Primary language Dutch
commented
Issue summary: View changes
Status: Active » Needs work
StatusFileSize
new feeds-optimize-feeds-queue-2978490-2.patch4.15 KB

This patch was posted by @andypost in #2820548-18: Fatal error when triggering Feeds via cron (it was called 2820548-interdiff-15.txt there) and can be used as a start for this issue.

MegaChriz credited andypost.

megachriz’s picture

megachriz
Primary language Dutch
commented
andypost’s picture

andypost
he/him
Primary language Russian
commented

Would be great to fit in a4

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#2820548: Fatal error when triggering Feeds via cron
megachriz’s picture

megachriz
Primary language Dutch
commented

@andypost
That would be great indeed. But because I would like to make a new release in the next few days and I think I won't have time to write the test before that, it is more likely to get in alpha5 than in alpha4.

mshdevx’s picture

mshdevx commented

Before applying this patch need to run update, which will delete all existing feeds queues with old logic of data storing. Because these queues will fail all the time and will never processed.

andypost’s picture

andypost
he/him
Primary language Russian
commented

I think it could be done in BC way, checking that queue item is integer (scalar) or feedItemInterface

mshdevx’s picture

mshdevx commented

@andypost, but then queues with objects will never processed and deleted from queue table. And each cron run will try to process them again

megachriz’s picture

megachriz
Primary language Dutch
commented
Issue summary: View changes
Issue tags: +Needs tests

@maxdev
I think @andypost is right and it can be done in a BC way without getting items stuck on the queue. It's already listed on the remaining tasks that this BC layer must be added.
It would be useful to write a test for this case too.

megachriz’s picture

megachriz
Primary language Dutch
commented
Issue summary: View changes

I've opened #3044983: Add test coverage for importing using multiple cron runs. I thought it was handier to split that task out of this one. Test coverage for that multiple cron runs case is probably handy to have for other issues as well.

We still need other automated tests here though, like one that checks if a full serialized feed entity can be processed (for backwards compatibility).

andypost’s picture

andypost
he/him
Primary language Russian
commented

Not clear where to add tests

megachriz’s picture

megachriz
Primary language Dutch
commented

@andypost
I think in tests/src/Kernel? Or in Drupal\Tests\feeds\Functional\QueueTest?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Next steps:

  • "Support both methods Test": Needs a test to show that when the object is still on the queue that that queued item will still be processed.
  • "May need to rewritten due to refactors"

ramil g made their first commit to this issue’s fork.

ramil g opened merge request !66

ramil g’s picture

ramil g commented
Status: Needs work » Needs review

This line was in testBeginStage() in FeedRefreshTest.php
$this->plugin->processItem(NULL);
I'm not sure what this was supposed to do, but I had to delete it because in the processItem function in FeedRefresh.php, it was giving me this error when running the test

TypeError: Drupal\feeds\FeedsExecutable::processItem(): Argument #3 ($params) must be of type array, null given, called in /var/www/html/web/modules/contrib/feeds/src/Plugin/QueueWorker/FeedRefresh.php on line 33

and part of the reason is that feedLoad was setup to always return a mock feed.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@ramil g and I looked at this last night, it is related to the patch, but we need to figure out where to either trigger or trip that error to occur in the new setup.

feeds/src/Entity/Feed.php:194 inside \Drupal\feeds\Entity\Feed::getType probably needs a spurious call to getType() at the start of the batch process or something.

megachriz’s picture

megachriz
Primary language Dutch
commented

Re #18

This line was in testBeginStage() in FeedRefreshTest.php
$this->plugin->processItem(NULL);

Taken from commit 238b4c15, this is what the test testBeginStage() originally looked like:

public function test() {
  $this->plugin->processItem(NULL);
  $this->plugin->processItem($this->feed);
}

And this is what FeedRefresh::processItem() looked like back then:

/**
 * {@inheritdoc}
 */
public function processItem($feed) {
  if (!$feed instanceof FeedInterface) {
    return;
  }

  try {
    $feed->lock();
  }
  catch (LockException $e) {
    // We don't really know when a queue item will execute, so it could be
    // locked which is ok.
    return;
  }
  $feed->clearStates();
  try {
    $this->dispatchEvent(FeedsEvents::INIT_IMPORT, new InitEvent($feed, 'fetch'));
    $fetch_event = $this->dispatchEvent(FeedsEvents::FETCH, new FetchEvent($feed));
    $feed->setState(StateInterface::PARSE, NULL);
  }
  catch (\Exception $exception) {
    return $this->handleException($feed, $exception);
  }

  $feed->saveStates();
  $this->queueFactory->get('feeds_feed_parse')->createItem([$feed, $fetch_event->getFetcherResult()]);
}

So I think that the test wanted the cover the case that the process is aborted gracefully when the value that is passed is not an object implementing FeedInterface. So from processItem() above, coverage for the following code:

if (!$feed instanceof FeedInterface) {
  return;
}

I think the line $this->plugin->processItem(NULL); should go into a separate test method and cover the code above: aborting when the feed is not an instance of FeedInterface.

megachriz’s picture

megachriz
Primary language Dutch
commented
Issue summary: View changes
Issue tags: -Needs tests

I think I've addressed all remaining tasks. If this passes tests, then this thing is ready for review!

andileco’s picture

andileco commented

I tested MR !66, and it worked well for me and caused no errors. +1 to committing.

  • MegaChriz committed 01a79a3 on 8.x-3.x authored by ramil g 
    Issue #2978490 by ramil g, MegaChriz, andypost, joelpittet, andileco:...
megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs review » Fixed

Thanks for testing, @andileco :).

This is now merged.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.