Bug #18926

class.tx_impexp.php sets 256m memory_limit without checking php.ini

Added by Sebastiaan Wiersema over 11 years ago. Updated over 6 years ago.

Status:
Closed
Priority:
Should have
Assignee:
Category:
Backend API
Target version:
-
Start date:
2008-06-09
Due date:
% Done:

0%

TYPO3 Version:
4.1
PHP Version:
5.1
Tags:
Complexity:
Is Regression:
Sprint Focus:

Description

There is no check in class.tx_impexp.php to see if it is necessary to alter the memory_limit setting. Even when the value is altered in php.ini to a higher value (say, 512MB) this is set back to 256MB.

I bumped into this when exporting a huge site. I tried to alter the php.ini memory_limit setting up to 512MB, which didn't have any effect. I still got a memory exceeded fatal error...

The following patch should do the trick:

--- typo3/sysext/impexp/class.tx_impexp.php 2007-12-14 15:08:55.000000000 0100
+
+ typo3/sysext/impexp/class.tx_impexp.php 2008-06-09 11:51:29.000000000 +0200
@ -180,7 +180,10 @
require_once (PATH_t3lib.'class.t3lib_refindex.php');

@ini_set('max_execution_time',600);
-@ini_set('memory_limit','256m');
$memory_limit = ini_get('memory_limit');
+if (strtolower(substr($memory_limit, -1)) == 'm' && substr($memory_limit, 0, -1) < 256) {
@ini_set('memory_limit','256m');
+}

(issue imported from #M8649)

8649.diff View (439 Bytes) Administrator Admin, 2009-02-27 16:23


Related issues

Related to TYPO3 Core - Bug #17045: T3D Export crashes with memory exhausted Closed 2007-02-27
Related to TYPO3 Core - Feature #27272: Configurable T3D export memory limit Closed 2011-06-07
Duplicates TYPO3 Core - Bug #17020: class.tx_impexp.php wants 256m memory_limit Closed 2007-02-20

Associated revisions

Revision 886cb1a5 (diff)
Added by Georg Ringer about 8 years ago

[BUGFIX] Remove hardcoded limits in T3D export

The memory_limit is hardcoded to 256m and
max_execution_time to 600.

There is no reason to hardcode those settings,
sometimes a bit more of both is needed.

Change-Id: I3e690eea1a079ed6cadd548230afc405e005b115
Resolves: #27272
Resolves: #17020
Resolves: #17045
Resolves: #18926
Releases: 4.5, 4.6
Reviewed-on: http://review.typo3.org/2610
Reviewed-by: Stefan Neufeind
Tested-by: Stefan Neufeind
Reviewed-by: Kay Strobach
Tested-by: Kay Strobach
Reviewed-by: Björn Pedersen
Reviewed-by: Steffen Gebert
Tested-by: Steffen Gebert

Revision 84774622 (diff)
Added by Georg Ringer almost 8 years ago

[BUGFIX] Remove hardcoded limits in T3D export

The memory_limit is hardcoded to 256m and
max_execution_time to 600.

There is no reason to hardcode those settings,
sometimes a bit more of both is needed.

Change-Id: I553c45c16be1ad561928678fa6a838abb78975d1
Resolves: #27272
Resolves: #17020
Resolves: #17045
Resolves: #18926
Releases: 4.5, 4.6
Reviewed-on: http://review.typo3.org/7063
Reviewed-by: Stefan Neufeind
Reviewed-by: Oliver Klee
Reviewed-by: Sebastian Fischer
Reviewed-by: Georg Ringer
Tested-by: Georg Ringer

History

#1 Updated by Benni Mack over 11 years ago

Hey Sebastiaan,

nice patch. Can you send it to the core newsgroup?

#2 Updated by Sebastiaan Wiersema over 11 years ago

I made a new patch. which addresses max_execution_time as well. I've already sent it to the typo3-dev list.

--- typo3/sysext/impexp/class.tx_impexp.php 2007-12-14 15:08:55.000000000 0100
+
+ typo3/sysext/impexp/class.tx_impexp.php 2008-06-10 12:17:01.000000000 +0200
@ -179,8 +179,34 @
require_once (PATH_t3lib.'class.t3lib_extfilefunc.php');
require_once (PATH_t3lib.'class.t3lib_refindex.php');

@ini_set('max_execution_time',600);
@ini_set('memory_limit','256m');
if (ini_get('max_execution_time') < 600) {
@ini_set('max_execution_time',600);
}
// Set the memory_limit to a specified minimum (in MB)
$memory_minimum = 256;
+$memory_limit = ini_get('memory_limit');
// -1 means no limit
if ($memory_limit != -1) {
switch (strtolower(substr($memory_limit, -1))) {
+ case 'g': // Gigabytes
+ // Minimum is 1GB, so no need to raise it
+ break;
+ case 'm': // Megabytes
+ if (substr($memory_limit, 0, -1) < $memory_minimum) {
+ @ini_set('memory_limit',$memory_minimum.'m');
+ }
+ break;
+ case 'k': // Kilobytes
+ if (substr($memory_limit, 0, -1) < ($memory_minimum * 1024)) {
+ @ini_set('memory_limit',$memory_minimum.'m');
+ }
+ break:
+ default: // Just bytes
+ if (ctype_digit($memory_limit) && $memory_limit < ($memory_minimum * 1024 * 1024)) {
+ @ini_set('memory_limit',$memory_minimum.'m');
+ }
+ }
+}

#3 Updated by Sebastiaan Wiersema over 11 years ago

Changed the patch according to some comments on the core list.

--- typo3_svn/typo3/sysext/impexp/class.tx_impexp.php 2008-06-10 12:41:18.000000000 0200
+
+ typo3_4.2/typo3/sysext/impexp/class.tx_impexp.php 2008-06-11 09:47:28.000000000 +0200
@ -179,8 +179,34 @
require_once (PATH_t3lib.'class.t3lib_extfilefunc.php');
require_once (PATH_t3lib.'class.t3lib_refindex.php');

@ini_set('max_execution_time',600);
@ini_set('memory_limit','256m');
if (ini_get('max_execution_time') < 600) {
@ini_set('max_execution_time',600);
}
// Set the memory_limit to a specified minimum in bytes
$memory_minimum = 256 * 1024 * 1024;
+$memory_limit = ini_get('memory_limit');
// -1 means no limit
if ($memory_limit != -1) {
switch (strtolower(substr($memory_limit, -1))) {
+ case 'g': // Gigabytes
+ // Minimum is 1GB, so no need to raise it
+ break;
+ case 'm': // Megabytes
+ if ((substr($memory_limit, 0, -1) * 1024 * 1024) < $memory_minimum) {
+ @ini_set('memory_limit', $memory_minimum);
+ }
+ break;
+ case 'k': // Kilobytes
+ if ((substr($memory_limit, 0, -1) * 1024) < $memory_minimum) {
+ @ini_set('memory_limit', $memory_minimum);
+ }
+ break;
+ default: // Just bytes
+ if (ctype_digit($memory_limit) && $memory_limit < $memory_minimum) {
+ @ini_set('memory_limit', $memory_minimum);
+ }
+ }
+}

#4 Updated by Peter Foerger over 10 years ago

After reading the discussion in the core list twice, I made a patch to remove the init_set() completely.

#5 Updated by Vladimir Podkovanov over 10 years ago

Hi Peter,
would you send the patch to core list? The trunc version still using that 256 limit.

#6 Updated by Stephan Seitz about 9 years ago

In my opinion, Peter is right and the ini_set() lines need to be removed completely.
Setting limits should never be part of a script or defined at some executed code at all. This is generally bad and ugly. Limits should always be defined "out-of-band". For this particular limits at php.ini, fastcgi parameters and/or ulimit settings. In general, a value of 256M or 10minutes of execution time is always a bad guess, as these values are individual and only project dependent. Even Sebastiaans patch which tries to avoid lowering limits can only be seen as a workaround and not a solution. Some things should definitely remain at system level and trying to consolidate them into a script will always result in errorrs which need to be worked around.
Some spice doesn't belong into soups, so PLEASE get off these ini_set() lines.

#7 Updated by Steffen Kamper about 9 years ago

ini_set is bad in general and shouldn't be used.
In this case we need a different way writing the data to file - sequentially, without needing so much memory.
I try to come with a better solution in 4.5 that solves all the memory hassles

#8 Updated by Martin Holtz about 9 years ago

It would be nice, if that bug could be fixed anyway.

If the rewrite needs more time and cannot get into 4.5, the ini_set() would stay at is it. So please, remove it now.

#9 Updated by Bernhard Kraft about 9 years ago

Any news on this?

Did you check my other bugnote with a proper fix for this problem;
http://bugs.typo3.org/view.php?id=5045

It solves the problem like explained in this message on the core list:
http://lists.typo3.org/pipermail/typo3-team-core/2008-July/017914.html

I do not have very much time. But if people think this is the right way and offer some of their time to test (and review) the patch I would invest some more time - because until now almost two workdays have been invested in this issue and it simply gets ignored.

#10 Updated by Stephan Seitz about 9 years ago

Bernhard, I really like your approach of writing chunks or segments or how you want to name it. Indeed, this should keep the memory footprint small even on huge exports. But I'm afraid, this won't get mature soon, so I'ld like to see the ini_set() statements removed as an interims solution anyway.

#11 Updated by Bernhard Kraft about 9 years ago

What do you mean by "not mature"? The code works and I used it for some quite large exports/imports without any problems?

The case is just there was quite a lot of "improve this, improve that" instead of something like "ok. let's use what you have and improve the rest later on" ...

I mean it works fully - when you read the whole thread I linked above you will notice that the concerns was: * The name of a variable * CGL issues * gzip compression

Martin Kutschker suggested I should compress each of the chunks on it's own and don't compress things like png's or other files which are already compressed - while my solution is to create the final T3D and run gzip over it (like it is currently the case).

So I stick to current behaviour, and made a fix for a problem. And my fix doesn't get accepted because some people suggest things I should also repair. But the memory issue and the handling of already compressed files are some completly DIFFERENT things.

greets,
Bernhard

#12 Updated by Bernhard Kraft about 9 years ago

Ok. I started updating the patch to current trunk version.
I will also add some unit-testing code so it is easier to check if everything works.
As soon as my updates are ready I will post them here and on the core list.

#13 Updated by Mr. Hudson about 8 years ago

Patch set 2 of change I3e690eea1a079ed6cadd548230afc405e005b115 has been pushed to the review server.
It is available at http://review.typo3.org/2610

#14 Updated by Steffen Gebert about 8 years ago

  • Category set to Backend API
  • Assignee changed from Bernhard Kraft to Georg Ringer
  • Target version changed from 0 to 4.6.0-beta1

#15 Updated by Georg Ringer about 8 years ago

  • Status changed from Accepted to Resolved

#16 Updated by Xavier Perseguers over 7 years ago

  • Status changed from Resolved to Closed

#17 Updated by Ernesto Baschny over 6 years ago

  • Target version deleted (4.6.0-beta1)

Also available in: Atom PDF