Getting the following error seemingly randomly when running imports on an AWS instance:
[Tue Mar 26 17:28:21 2013] [error] [client x.x.x.x] PHP Fatal error: Nesting level too deep - recursive dependency? in htdocs/sites/all/modules/contrib/feeds/plugins/FeedsProcessor.inc on line 199, referer: /batch?op=start&id=365
Looking at the code, the line that breaks it is:
$message .= '<pre>' . var_export($entity, TRUE) . '</pre>';
This fatal error prevents the exception from being logged, so we never really see what the root cause of the problem is.
We're only seeing this error on our AWS instance which we've tried to identically configure to our internal test server which does not exhibit this behaviour. We're now at the point where we seem to have the Apache/PHP identical and we're looking into potential differences in the system level configuration for how the AMI might be configured differently.
Despite all this however, var_export seems to be somewhat known to cause this type of error as are var_dump and print_r. Might I recommend drupal_var_export? It seems that devel's kprint_r utilizes drupal_var_export and is circular safe.
Alternatively if anybody is aware of how to configure AWS to avoid this then I'd also be very grateful for that :)
Thanks!
Comment | File | Size | Author |
---|---|---|---|
#15 | feeds-exception-log-1953008-15.patch | 3.88 KB | klausi |
#10 | feeds-var-export-error-1953008-10.patch | 2.06 KB | twistor |
Comments
Comment #1
carn1x CreditAttribution: carn1x commentedWell actually I tried hacking in the drupal_var_export() to see if that does the job but it still generates a similar error. However I'm wondering if there is another way to prevent this? Obviously something is a bit wrong with our AWS instance although it's a pretty vanilla VM, so I'll change this to a support request.
Comment #2
MegaChriz CreditAttribution: MegaChriz commentedI encountered the exact same problem when working on a processor plugin for Ubercart Addresses. The entity contains a cross reference, resulting in the "Nesting level too deep" error. The easiest way I see to fix this is to move generating the error to a separate method, so other classes have a chance to override the method. Of course, any processor plugin that deals with classes that can have cross references should override this method too.
The attached patch moves the code for creating the error message to a new method called
createLogMessage()
.Comment #3
MegaChriz CreditAttribution: MegaChriz commentedOops, I had set the parameters in the wrong error when calling the
createLogMessage()
method:+ $message = $this->createLogMessage($e, $item, $entity);
Should have been:
+ $message = $this->createLogMessage($e, $entity, $item);
New patch.
Comment #4
twistor CreditAttribution: twistor commentedI've gone ahead and committed this to 7.x-2.x. Anything that makes the process() method shorter is good to me. I also switched to drupal_var_export() because it makes nicer exports. Not sure if it will help though.
Comment #5
twistor CreditAttribution: twistor commentedComment #6
MegaChriz CreditAttribution: MegaChriz commentedThanks for committing the patch!
Other solutions?
Meanwhile, I had been looking for better solutions for this issue, though I have not found one yet. I was looking for a way to move the whole contents of
try
into a method calledprocessItem()
. This way classes could override this method, call the parent and decide if they want to catch the Exception themselves (see pseudo code below for an example). The caveat is that a lot of variables defined before thetry
statement are used inside thetry
, thus that would mean a lot of parameters to pass for theprocessItem()
method. Maybe something like an$options
parameter would do the trick, though that would be a bit dirty as the variables are not really options.Pseudo code:
drupal_var_export()
drupal_var_export()
does a better job for stdClass instances, but not for instances of other classes: it won't be a direct solution for objects with cross references.Backport not needed?
It looks like that there is no need for a backport to 6.x-1.x, at least not yet, because the
process()
method in the 6.x-1.x version is abstract, so basically there is nothing from this issue to backport at this point.Comment #7
carn1x CreditAttribution: carn1x commentedI discovered that suppressing the PHP Fatal error actually prevents data from getting imported. If the PHP Fatal error occurs then the row is never marked as failed importing, and the batch is still ready for import on the next attempt either with BatchAPI or on cron. As a result in order to maintain data integrity I'm sticking with the PHP Fatal error for now.
Comment #8
MegaChriz CreditAttribution: MegaChriz commentedComment #9
twistor CreditAttribution: twistor commentedI'm bumping this back up. I think we can kill a whole class of bug reports if we just fix this.
Comment #10
twistor CreditAttribution: twistor commentedThis makes the log much more usable, plus it escapes the output, which we should be doing already.
Comment #11
twistor CreditAttribution: twistor commentedThis issue is related. The nesting level issue can cause Drupal to not release the lock, so trying to import again breaks.
Comment #12
klausiThe problem with this approach is that the variables are still concatenated directly into the message string. That message string can become quite large and pollutes the locales_source table since it is run through t() on display. So you get lots of source translation variants in your translation system, which is bad. The message should be a static string with placeholders, I propose that in #2502419: Log messages XSS attack vector.
Comment #13
MegaChriz CreditAttribution: MegaChriz commentedI agree with klausi that we shouldn't put the whole message, including exception and var export in one string to log. The patch in #10 should be merged with the patch from #2502419: Log messages XSS attack vector. This will require an API change, since the return signature for the
createLogMessage()
will change from string to array (see the patch from the other issue). We should take in account though that modules that override that method may still return a string (Ubercart Addresses does for example, though since I control that module I can easily adjust that once this issue is fixed).There are also other places where an exception message is directly logged. Fixing these is beyond the scope of this issue, we should handle that in #2364103: Feeds error log crashes when log messages are too long.
Comment #14
twistor CreditAttribution: twistor commentedI think we should create a new method that has the correct return variables and deprecate createLogMessage(). A security release that breaks things will make it harder to adopt. A polluted locales_source table is minor compared to someone avoiding the update because it breaks their processor. Custom implemented or otherwise.
Comment #15
klausiYep, we could do that, patch attached.
Comment #18
twistor CreditAttribution: twistor commentedWe need sanitizing in 6.x as well.
Comment #19
twistor CreditAttribution: twistor commentedErr, other issue.