On PHP 5.2.13 I'm encountering a big memory leak when loading and rendering a list of nodes one by one. In the test PHP script I've included below, the node object should be getting freed when the function ends and goes out of scope. Instead however it seems to remain in memory.

I believe this is due to circular references being created in content.module. For example in the element array around line 760 where '#node' is set to point to the $node object ('#node' => $node). If I change each instance to '#node' => drupal_clone($node) the memory leak goes away. This also makes the 3rd parameter to node_load (telling drupal to reset the cache) effectively useless.

I don't know if this problem is CCK specific but I'm not sure where else to submit to and for my particular usage, it only occurs when I have CCK module enabled. This issue is affecting other modules causing PHP to croak thru excessive memory use, including Sphinx Search. See: http://drupal.org/node/329831 


 require 'includes/bootstrap.inc';
 drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);


for($nid = 1; $nid < 2000; $nid++) {
  if (nl($nid)) {
    print memory_get_usage()/1024/1024 . "\n";
    usleep(50000);
  }
}

function nl($nid) {
  $node = node_load($nid, NULL, TRUE);

  if ($node) {
    $node = node_build_content($node, FALSE, FALSE);
    $node->body = drupal_render($node->content);

    return 1;
  }

  return 0;
}
CommentFileSizeAuthor
#25 cck_token_memory_leak-736440-25.patch1.25 KByched
#21 cck_token_memory_leak-736440-21.patch1.25 KByched
#15 cck_token_memory_leak-736440.patch1.63 KByched
#4 cck_clone_node.patch2.25 KBdhthwy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dhthwy’s picture

dhthwy CreditAttribution: dhthwy commented
Title: Memory Leaks » CCK Provokes PHP memory leak with recursive objects

You can find the same problem happening with the Pathauto/Token: http://drupal.org/node/648266

I'm updating title to reflect.

andreiashu’s picture

andreiashu CreditAttribution: andreiashu commented

Hi dhthwy,
I also experienced the problem with Pathauto/Token and the patch provided in the issue seemed to alleviate the problem. This raises the question if the problem is only Token related, CCK related or both. With your test above, did you have token module enabled?

dhthwy’s picture

dhthwy CreditAttribution: dhthwy commented

No it is a new Drupal site and I hadn't gotten around to Token yet. I'm sure Token, CCK, and many other modules have this problem.

To find the leak I disabled all non-core modules until the leak disappeared and then re-enabled each module one by one.

Cloning the node object instead of creating a circular reference to the parent node object fixes the memory leak though.

dhthwy’s picture

dhthwy CreditAttribution: dhthwy commented
Status: Active » Needs review
FileSize
cck_clone_node.patch2.25 KB

Ok this patch fixed it for me.

Copy the node object instead of referencing it where array '#node' => $node for PHP5 so we don't get circular references and thus PHP is able to free the node object when it's finished with it. It gets copied anyway in PHP4 and CCK works fine for PHP4 right?

thekenshow’s picture

thekenshow CreditAttribution: thekenshow commented

Tested the patch in #4 on D6.16 and it's working great. I've been fighting with memory limits using migrate to import into nodes with cck fields. I was running out of memory (300 Mb allocated) after 500 imports, and I just imported 2300 nodes with no problem. Many thanks!

geodaniel’s picture

geodaniel CreditAttribution: geodaniel commented
Status: Needs review » Reviewed & tested by the community

The patch solved the memory leak issue for me as well. Do any other parts of the CCK code rely on the #node element being recursive?

stella’s picture

stella CreditAttribution: stella commented
Version: 6.x-2.6 » 6.x-2.7
Status: Reviewed & tested by the community » Needs review

subscribe

omerida’s picture

omerida CreditAttribution: omerida commented

subscribe

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented

This patch solved a lot of memory leaks for us while using the migrate module.
Are you considering committing this upstream?

yched’s picture

yched CreditAttribution: yched commented

looking at this right now

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented

yched, do you need some help or statistics?

We ran a migration of over 2 million nodes +- and with this patch + some little tweaks to custom code we manage to get a stable memory usage. Without we are in big trouble.

yched’s picture

yched CreditAttribution: yched commented

No need, I've seen the difference myself on a couple imports we're running on a client project.

The impact on migration is when generating tokens for the saved nodes (e.g pathauto or whatever). CCK's token integration is a bit funky, and actually renders nodes to get token values. Other than that, creating / updating nodes causes triggers no node render.

The fix could simply consist in cloning the $node at the top of content_token_values(). I'm a little wary of altering the whole render pipeline for this. Other than token generation, there are not much use cases for rendering thousands of nodes in a single request.

In short : I haven't had time yet to look at this in-depth, and haven't decided yet between the two approaches. One of them will eventually get in soonish :-)

Sylvain_G’s picture

Sylvain_G CreditAttribution: Sylvain_G commented

subscribe

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
CreditAttribution: roderik commented

@yched: I just wanna voice support for cloning $node at the top of content_token_values(). For a different reason than memory leaks.

Right now, any hook_token_values() implementation that gets called after content_token_values(), does not have access to already-rendered $node->content anymore.
Which is problematic for the body field.. because you cannot just call node_build_content() again to re-render the body. (A search for 'node_prepare twice' will give pointers why.) This apart from the fact that it's a waste of resources in such a potentially-often-called piece of code...

And if the token module would get rid of the drupal_clone() call they are doing 'for protection' (as they are considering)... calling functions would be in trouble too.

I don't know how important this is in the big picture... but it seems like 'the right thing' from my viewpoint. (I'm trying to fix an issue in notifications_content_token_values(), which happens to get called after content_token_values(). in #736328: render the node more completely for [node-body] to better handle cck fields (and others))
So, just wanted to chime in.

yched’s picture

yched CreditAttribution: yched commented
Status: Needs review » Fixed
FileSize
cck_token_memory_leak-736440.patch1.63 KB

Committed the attached patch to the 6--2 and 6--3 branches

franz’s picture

franz
any
Location Montréal
CreditAttribution: franz commented
Status: Fixed » Needs work

When working with migration, I had an error accusing a __clone called on a non-object. Turned out it needs a ternary evaluation around line 1805

'#node' => is_object($node) ? drupal_clone($node) : $node,

yched’s picture

yched CreditAttribution: yched commented
Status: Needs work » Fixed

@franz : it seems your remark applies to the patch proposed in #4.
The fix I just committed doesn't affect '#node' properties, but only acts on content_token_values().

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
CreditAttribution: roderik commented

*commences overstepping boundaries*

As long as you're going to prompt a new -dev tarball for 6.x first time in a month...
could you possibly commit the small, completely obvious -to core developers- and unintrusive PHP5.3 fix from #705512: drupal_alter('form'... called from content_add_more_js needs to pass $form_state as a reference? Comment #1 and #21 tell the whole story.

It happens to be at the top of the queue right now. And the 'assigned' property is outdated.

yched’s picture

yched CreditAttribution: yched commented

@roderik : I'll look at that patch. I know the commit rate on CCK is fairly low currently :-/, but let's not add noise in completely unrelated issues.

yched’s picture

yched CreditAttribution: yched commented
Status: Fixed » Active

Reopening. #988890: Fatal error in content.token.inc when I click on "add new comment" (marked as dupe) reports that my commit causes fatal errors when posting comments : "__clone method called on non-object in content.token.inc on line 30"

yched’s picture

yched CreditAttribution: yched commented
FileSize
cck_token_memory_leak-736440-21.patch1.25 KB

I am not able to reproduce the error reported in #988890: Fatal error in content.token.inc when I click on "add new comment", but I committed a fix that preventively moves the 'clone' call a little below to prevent from funky params being received.

Final patch attached.

yched’s picture

yched CreditAttribution: yched commented
Status: Active » Fixed

let's hope this is really fixed now.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
CreditAttribution: roderik commented

Thanks, much appreciated.
I was looking into the same thing, but just seeing your CVS commits when rolling the same patch.

(For completeness:
also tested on CCK2 & CCK3, with no ability to produce the error. So I'm assuming something passed NULL as $node.)

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
CreditAttribution: roderik commented

*cough* ehm, I don't want to nitpick unnecessarily... but to prevent more obscure issues / since the term "PHP4" gets mentioned in this issue, apparently CCK still works on it?

Then 'clone $node' should be 'drupal_clone($node)'?

yched’s picture

yched CreditAttribution: yched commented
FileSize
cck_token_memory_leak-736440-25.patch1.25 KB

@roderik: doh, good call. Fixed, thanks !

Updated patch, for history's sake.

J3’s picture

J3 CreditAttribution: J3 commented

Seems this patch made my reference CCK field (user/node reference) tokens work.

However, all the fields are not update to date. For example, I changed a CCK field named "status" via comment (comment driven) and sent a mail including "status" token. But the "status" is old status, not the one after modification.

Is this issue due to the "clone" (drupal_clone)?

yched’s picture

yched CreditAttribution: yched commented

I don't really see how this could be related. Off-hand, I'd be more inclined to relate that to comment_driven.
Did it work before the clone and the CCK change ? (you can simply try to remove the drupal_clone() call on you local setup and see if it works better)

J3’s picture

J3 CreditAttribution: J3 commented

um, seems you are right. I replaced drupal_clone() with =, and got same problem...
Thanks.

Status: Fixed » Closed (fixed)

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

beck24’s picture

beck24 CreditAttribution: beck24 commented
Version: 6.x-2.7 » 6.x-3.0-alpha3

Is this actually fixed?

I see that the last patch listed here made it into content.token.inc but the proposed patch from #4 did not make it into content.module, or modules/fieldgroup/fieldgroup.module

I realize this doesn't affect the vast majority of sites, including many of my own, but this most recent project has a lot of data.
I'm facing extreme memory leaks while trying to change field settings on content types that have node references - the problem being that I have over 80k nodes of one type, that are referenced by ~5k nodes of another type. There's definitely some recursion happening and it's running out of memory even if I set it to 1G.

Those patches are way out of date, but the concept should be the same. I'll try drupal_clone() for the recursive references and report back any changes.
In the meantime - is there a reason #4 didn't make it into the official build?

beck24’s picture

beck24 CreditAttribution: beck24 commented

After testing - patch #4 didn't clear anything up.
2 days of pulling out my hair and I've found that the issue was caused by the conditional fields module which apparently has terrible scalability.

lambic’s picture

lambic CreditAttribution: lambic commented
Version: 6.x-3.0-alpha3 » 6.x-2.9
Status: Closed (fixed) » Active

I'm using 2.9 and I'm getting memory leakage problems on content_token_values, but it seems to be happening at the content_view() and drupal_render() steps. I'm. After each call to content_token_values, my memory consumption is increased by about 200k.

yonailo’s picture

yonailo CreditAttribution: yonailo commented

Yes, I can confirm that I have also the same problem as #32, and is there a reason #4 didn't make it into the official build?