I ran in to this whilst trying to use the --file= option in the Drush integration patch #608408: Drush integration for Feeds with the CSV importer.

pushImport() is using a strict !== comparison against FEEDS_BATCH_COMPLETE (value 1.0) but in my case the output of FeedsState::progress is integer 1, so it gets stuck in the do { } loop.

Casting the progress value to float in FeedsState::progress ($this->progress = (float) $progress / $total) fixes my issue, but I'm unsure if this is the correct fix, given that importing the same file via the feeds UI works fine...

FeedsSource::import() is not using a strict comparison:

    $result = $this->progressImporting();
    if ($result == FEEDS_BATCH_COMPLETE || isset($e)) {

although, scheduleImport() is - I haven't looked to see if that breaks too.

    elseif ($this->progressImporting() === FEEDS_BATCH_COMPLETE) {
      JobScheduler::get('feeds_source_import')->set($job);
CommentFileSizeAuthor
#6 feeds-pushimport-loop-2624344-6.patch1.66 KBMegaChriz
#6 feeds-pushimport-loop-2624344-6-test-only.patch1.18 KBMegaChriz
#5 feeds-pushimport_loop-2624344-5.patch2.44 KBgrahamC
#2 feeds-pushimport_loop-2624344-2.patch860 bytesgrahamC
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

grahamC created an issue. See original summary.

grahamC’s picture

grahamC
Location Oxford, UK
CreditAttribution: grahamC at Webcurl Ltd commented
FileSize
feeds-pushimport_loop-2624344-2.patch860 bytes

Attach patch with casting fix

controla’s picture

controla CreditAttribution: controla commented

Patch works, feed-import with --file finished correctly at the end of the batch.
Thank you!

MegaChriz’s picture

MegaChriz CreditAttribution: MegaChriz at WebCoo commented
Status: Active » Needs review

Setting status to "Needs review" so that the patch is evaluated by the testbot.

Can the bug be demonstrated with an automated test?

grahamC’s picture

grahamC
Location Oxford, UK
CreditAttribution: grahamC as a volunteer and at Webcurl Ltd commented
FileSize
feeds-pushimport_loop-2624344-5.patch2.44 KB

Added a test which makes it loop - looks like the behaviour may be unique to the CSV parser, as this is the only one I can see calling FeedsState::progress()

MegaChriz’s picture

MegaChriz CreditAttribution: MegaChriz at WebCoo commented
FileSize
feeds-pushimport-loop-2624344-6-test-only.patch1.18 KB
feeds-pushimport-loop-2624344-6.patch1.66 KB

Thanks for the automated test! I ran the test locally without the fix applied and it kept running. After examining what was happening in the test, the fix looks fine to me.

I've also examined the history of Feeds of why the comparison is strict. It seems to be added in the following commit:
http://cgit.drupalcode.org/feeds/commit/?id=f84c73b129eefd0291c7e3b5aeeb...
Back then it seems to make sense because FeedsSource::progressImporting() could return exactly FEEDS_BATCH_COMPLETE.

There seem to be indeed some inconsistenties in the use of comparing values against FEEDS_BATCH_COMPLETE, but honestly I do not find it worth my time to sort that out completely (there is enough other stuff to do for Feeds).

In the attached patch I have moved the test to FeedsRSStoNodesTest, in order to keep related tests together as much as possible. Perhaps it would be better to move these tests out of the test case for node processor as the test doesn't specifically tests the processor's behaviour, but that would be out of scope for this issue. Also added a test-only patch to ensure it fails on the testbot (I wonder how the testbot would deal with a patch that has and endless loop?).

The last submitted patch, 6: feeds-pushimport-loop-2624344-6-test-only.patch, failed testing.

The last submitted patch, 6: feeds-pushimport-loop-2624344-6-test-only.patch, failed testing.

  • MegaChriz committed dd038fe on 7.x-2.x authored by grahamC 
    Issue #2624344 by grahamC, MegaChriz: Import via pushImport() keeps...
MegaChriz’s picture

MegaChriz CreditAttribution: MegaChriz at WebCoo commented
Status: Needs review » Fixed

Now that I see patches failing in #2450365: Importer is not rescheduled for new sources when using attach to node, #2630694: Run background jobs directly in queue. and #2174841: Call FeedsSource::schedule() after manual import. on the same error as the endless loop patch here, I wonder if the fix provided here would solve a lot of reported cron issues too (especially the ones about import hanging at x%, never completes)? See #1995728: [META] Cron import not working on 7.x-2.0-alpha8 and later. If that is the case, then big thanks for finding and fixing this bug, grahamC! (If it is not, then still big thanks :D).

Committed #6.

Status: Fixed » Closed (fixed)

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

nicxvan’s picture

nicxvan CreditAttribution: nicxvan as a volunteer commented
Status: Closed (fixed) » Needs work

I'm not sure if this should be a new issue:

I'm getting the infinite loop when calling the feed-import programmatically. The only difference is I'm not passing the -file parameter, I preset the url so the command looks something like this:

/home/nic/.composer/vendor/bin/drush @sites feeds-import feed_name -y 2>&1

It then loops for about 90 seconds before saying failed to import x nodes.

nicxvan’s picture

nicxvan CreditAttribution: nicxvan as a volunteer commented
Status: Needs work » Closed (fixed)

I have done some more investigation. Other imports work, I get a SQLSTATE[HY000]: General error: 1205 Lock wait timeout exceeded; try restarting transaction for one particular import. I will close this and after further testing and open a new issue.