This issue is really another symptom of #2912130: Missing temporary files in load balanced environments. When the parser is unable to access the fetched file, FetcherResult::checkFile() throws a RuntimeException. But also ties into #3155929: Http Fetcher receiving 304s Do not clear out their queue items. where this is also a way thrown exceptions aren't caught, leaving un-processable items in the Queue.
At this moment, I am seeing this happen via the feeds_ex module's XMLParser (TODO verify). But I assume this might be an issue for any Parser. The uncaught exception is problematic, because it prevents queue items from being removed from the queue. This can get really problematic due to
Do the RSS, or other parsers, catch these exceptions? Where/how should RuntimeException be caught?
FeedsExecutable::handleException() uses:
if ($exception instanceof \RuntimeException) {
$this->messenger->addError($exception->getMessage());
return;
}
(should be "\RuntimeException")
But FeedsQueueExecutable::handleException() does not handle RuntimeException in this same way. It just checks for an EmptyFeedException and returns. I'm not sure on the rationale for using RuntimeException here over anything else. Maybe it makes sense to throw a core FileNotFoundException and catch that instead?
Edit: I think it's more accurate to say that the Exception is caught, but is then re-thrown, and never caught.
Edit 2: for comparison, CSVParser throws an InvalidArgumentException. And this may actuallybelong more in feeds_ex, since it's in ParserBase::parse() where he exception is thrown, caught, and th-thrown (the first time).
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | feeds-3158678-9-catch-runtime-errors.patch | 2.58 KB | steven jones |
| #5 | feeds-runtime-exception-3158678-5.patch | 562 bytes | bburg |
Show commandsStart 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:
changes
Comments
Comment #2
bburgComment #3
bburgComment #4
bburgComment #5
bburgIn the meantime, I just need these things to work, or at minimum, cut down the noise of errors. Here's a patch for adding to one's composer files.
Comment #6
megachrizIt is very much possible that I missed a few things when I refactored the import process.
Looking at the code it does look strange that a RuntimeException is catched during an import in the UI and not during cron. Before the refactoring this was also the case. I guess I just copy-pasted that behavior without paying attention to why the difference in handling exceptions existed.
It would be cool if we had kernel/functional tests for this issue.
Comment #7
megachrizMarking as stable release blocker.
Comment #8
bburgI think one problem with my patch is that there is no log message for the missing file.
Comment #9
steven jones commentedHere's a patch that moves this on a little, by adding that logging that was missing in #5 and should be re-rolled for latest dev.
Comment #11
megachrizI do wonder if we should catch these exceptions and abort the import process. In some cases a resource could be temporary unavailable and in such case you rather want Feeds to retry at a later time instead of aborting the process. Maybe Feeds should record how often it retries and only abort the process after a certain amount of retries? We could record this into a state variable.
Comment #13
megachrizIt has been a while since I last worked on this issue, but in the branch 3158678-tests_only I had made a draft for a test. I couldn't post it before because it depended on the changes from #2912130: Missing temporary files in load balanced environments. I'm posting it now and plan to look into it in more detail later.
Comment #14
megachrizI took a brief look at this issue today. I think something like the proposed fix looks okay, but I think we should use
watchdog_exception()and the equivalent of that in Drupal 10.I do wonder the following:
Comment #15
megachrizI think that #3372368: in_progress filesystem grows indefinitely is closely related. To try to move this issue forward, here's what I plan to do:
watchdog_exception()(+ the equivalent of that for Drupal 10)There is however a risk of introducing a regression with the proposed fix: some imports that now go through after a temporary error may then instead get aborted. But maybe we should see if any issue reports come up after making a change?
It is about the following type of situation: an import is running on cron, but during the import a resource becomes unavailable due to a hitch.
However, it looks like the situation where an error happens that is not temporary is way more likely to happen. In this case, Feeds keeps trying over and over again and that makes the import to never finish. In case of #3372368: in_progress filesystem grows indefinitely it is even worse: a file to import gets downloaded and on the next try it gets downloaded again. And again. Resulting into a huge pile of files over time.
Thoughts on this issue are welcome! Hopefully I can get the steps outlined here done soon.
Comment #18
megachrizComment #19
x775 commentedHi @MegaChriz
We are still seeing errors similar to
`RuntimeException: File /tmp/feeds_http_fetcherhNFdBE does not exist. in Drupal\feeds\Result\FetcherResult->checkFile() (line 53 of feeds/src/Result/FetcherResult.php).`
despite running 8.x-3.0-beta4.
Do we need to update?
Thanks!
Comment #20
megachriz@x775
It's possible that some items are still left in the queue. The changes in 8.x-3.0-beta4 only aimed to fix problems like you are having for new imports. However, the code changes on this issue should fix it also for "left over" tasks. Did you try the code from this issue?
Comment #21
megachrizNow that #3372368: in_progress filesystem grows indefinitely and #3080098: Delete Empty Files Created If Empty/Error Exception is Returned are done, I could perhaps give this issue another look.
Comment #22
megachrizI reduced the scope of the issue this is fixing. This reduces the chance of regressions, which was I afraid for, but it’s also possible that doesn’t fix the reported issue entirely.
Now an import gets aborted only when the file to import (which is saved in the "in_progress" directory) no longer exists. When Feeds fails to fetch a resource, for example because of a 404, Feeds will not abort the import process on cron runs. Instead, Feeds will retry the fetch on the next cron. It does so forever, which is an issue as well, but I think we should address that in #3312064: Feed fetch errors don't seem to be handled gracefully when it encounters a 404 instead.
By doing so, we can at least get a part of the issue fixed and at the same time reduce the chance of regressions.
Comment #24
megachrizI merged the code!