From 70fe0928675d1f1080a9d6496f549baf5ae482a9 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Tue, 14 Sep 2021 13:44:02 +0200 Subject: [PATCH] Simplify memoryIniValueToBytes, tweak code to use less memory overall --- src/Monolog/Handler/StreamHandler.php | 28 ++++----- src/Monolog/Utils.php | 36 ++++-------- tests/Monolog/Handler/StreamHandlerTest.php | 63 +++++++-------------- tests/Monolog/UtilsTest.php | 24 ++++---- 4 files changed, 54 insertions(+), 97 deletions(-) diff --git a/src/Monolog/Handler/StreamHandler.php b/src/Monolog/Handler/StreamHandler.php index 8d118928..ec2519c1 100644 --- a/src/Monolog/Handler/StreamHandler.php +++ b/src/Monolog/Handler/StreamHandler.php @@ -26,9 +26,7 @@ use Monolog\Utils; class StreamHandler extends AbstractProcessingHandler { /** @const int */ - const SAFE_MEMORY_OFFSET = 1024; - /** @const int */ - const MAX_CHUNK_SIZE = 2147483647; + protected const MAX_CHUNK_SIZE = 100 * 1024 * 1024; /** @var int */ protected $streamChunkSize = self::MAX_CHUNK_SIZE; /** @var resource|null */ @@ -55,25 +53,21 @@ class StreamHandler extends AbstractProcessingHandler { parent::__construct($level, $bubble); - if ($phpMemoryLimit = ini_get('memory_limit')) { - if (($memoryInByes = Utils::memoryIniValueToBytes($phpMemoryLimit))) { - $memoryUsage = memory_get_usage(true); - if (($memoryInByes - $memoryUsage) < $this->streamChunkSize) { - $this->streamChunkSize = $memoryInByes - $memoryUsage - self::SAFE_MEMORY_OFFSET; - } + if (($phpMemoryLimit = Utils::expandIniShorthandBytes(ini_get('memory_limit'))) !== false) { + if ($phpMemoryLimit > 0) { + // use max 10% of allowed memory for the chunk size + $this->streamChunkSize = max((int) ($phpMemoryLimit / 10), 10*1024); } + // else memory is unlimited, keep the buffer to the default 100MB + } else { + // no memory limit information, use a conservative 10MB + $this->streamChunkSize = 10*10*1024; } if (is_resource($stream)) { $this->stream = $stream; - try { - stream_set_chunk_size($this->stream, $this->streamChunkSize); - } catch (\Exception $exception) { - throw new \RuntimeException('Impossible to set the stream chunk size.' - .PHP_EOL.'Error: '.$exception->getMessage() - .PHP_EOL.'Trace: '.$exception->getTraceAsString()); - } + stream_set_chunk_size($this->stream, $this->streamChunkSize); } elseif (is_string($stream)) { $this->url = Utils::canonicalizePath($stream); } else { @@ -119,7 +113,7 @@ class StreamHandler extends AbstractProcessingHandler /** * @return int */ - public function getStreamChunkSize() : int + public function getStreamChunkSize(): int { return $this->streamChunkSize; } diff --git a/src/Monolog/Utils.php b/src/Monolog/Utils.php index 28a7f334..d3e7ad09 100644 --- a/src/Monolog/Utils.php +++ b/src/Monolog/Utils.php @@ -229,41 +229,27 @@ final class Utils /** * Converts a string with a valid 'memory_limit' format, to bytes. - * Reference: Function code from https://www.php.net/manual/en/function.ini-get.php - * @param string|int $val + * + * @param string|false $val * @return int|false Returns an integer representing bytes. Returns FALSE in case of error. */ - public static function memoryIniValueToBytes($val) + public static function expandIniShorthandBytes($val) { - if (!is_string($val) && !is_integer($val)) { + if (!is_string($val)) { return false; } - $val = trim((string)$val); + // support -1 + if ((int) $val < 0) { + return (int) $val; + } - if (empty($val)) { + if (!preg_match('/^\s*(?\d+)(?:\.\d+)?\s*(?[gmk]?)\s*$/i', $val, $match)) { return false; } - $valLen = strlen($val); - $last = strtolower($val[$valLen - 1]); - - if (preg_match('/[a-zA-Z]/', $last)) { - if ($valLen == 1) { - return false; - } - - $val = substr($val, 0, -1); - } - - if (!is_numeric($val) || $val < 0) { - return false; - } - - //Lets be explicit here - $val = (int)($val); - - switch ($last) { + $val = (int) $match['val']; + switch (strtolower($match['unit'] ?? '')) { case 'g': $val *= 1024; case 'm': diff --git a/tests/Monolog/Handler/StreamHandlerTest.php b/tests/Monolog/Handler/StreamHandlerTest.php index 8a9ae184..275aa955 100644 --- a/tests/Monolog/Handler/StreamHandlerTest.php +++ b/tests/Monolog/Handler/StreamHandlerTest.php @@ -225,15 +225,15 @@ class StreamHandlerTest extends TestCase public function provideMemoryValues() { return [ - ['1M', true], - ['10M', true], - ['1024M', true], - ['1G', true], - ['2000M', true], - ['2050M', true], - ['2048M', true], - ['3G', false], - ['2560M', false], + ['1M', (int) (1024*1024/10)], + ['10M', (int) (1024*1024)], + ['1024M', (int) (1024*1024*1024/10)], + ['1G', (int) (1024*1024*1024/10)], + ['2000M', (int) (2000*1024*1024/10)], + ['2050M', (int) (2050*1024*1024/10)], + ['2048M', (int) (2048*1024*1024/10)], + ['3G', (int) (3*1024*1024*1024/10)], + ['2560M', (int) (2560*1024*1024/10)], ]; } @@ -241,52 +241,28 @@ class StreamHandlerTest extends TestCase * @dataProvider provideMemoryValues * @return void */ - public function testPreventOOMError($phpMemory, $chunkSizeDecreased) + public function testPreventOOMError($phpMemory, $expectedChunkSize) { - $memoryLimit = ini_set('memory_limit', $phpMemory); + $previousValue = ini_set('memory_limit', $phpMemory); - if ($memoryLimit === false) { - /* - * We could not set a memory limit that would trigger the error. - * There is no need to continue with this test. - */ - - $this->assertTrue(true); - return; + if ($previousValue === false) { + $this->markTestSkipped('We could not set a memory limit that would trigger the error.'); } $stream = tmpfile(); if ($stream === false) { - /* - * We could create a temp file to be use as a stream. - * There is no need to continue with this test. - */ - $this->assertTrue(true); - return; + $this->markTestSkipped('We could not create a temp file to be use as a stream.'); } $exceptionRaised = false; - try { - $handler = new StreamHandler($stream); - stream_get_contents($stream, 1024); - } catch (\RuntimeException $exception) { - /* - * At this point, stream_set_chunk_size() failed in the constructor. - * Probably because not enough memory. - * The rest of th test depends on the success pf stream_set_chunk_size(), that is why - * if exception is raised (which is true at this point), the rest of assertions will not be executed. - */ - $exceptionRaised = true; - } + $handler = new StreamHandler($stream); + stream_get_contents($stream, 1024); - ini_set('memory_limit', $memoryLimit); - $this->assertEquals($memoryLimit, ini_get('memory_limit')); + ini_set('memory_limit', $previousValue); - if (!$exceptionRaised) { - $this->assertEquals($chunkSizeDecreased, $handler->getStreamChunkSize() < StreamHandler::MAX_CHUNK_SIZE); - } + $this->assertEquals($expectedChunkSize, $handler->getStreamChunkSize()); } /** @@ -297,8 +273,7 @@ class StreamHandlerTest extends TestCase $previousValue = ini_set('memory_limit', '2048M'); if ($previousValue === false) { - $this->assertTrue(true); - return; + $this->markTestSkipped('We could not set a memory limit that would trigger the error.'); } $stream = tmpfile(); diff --git a/tests/Monolog/UtilsTest.php b/tests/Monolog/UtilsTest.php index 95d11fa5..aced3baf 100644 --- a/tests/Monolog/UtilsTest.php +++ b/tests/Monolog/UtilsTest.php @@ -142,16 +142,18 @@ class UtilsTest extends \PHPUnit_Framework_TestCase ]; } - public function provideMemoryIniValuesToConvertToBytes() + public function provideIniValuesToConvertToBytes() { return [ ['1', 1], ['2', 2], ['2.5', 2], ['2.9', 2], - ['1B', 1], - ['1X', 1], + ['1B', false], + ['1X', false], ['1K', 1024], + ['1 K', 1024], + [' 5 M ', 5*1024*1024], ['1G', 1073741824], ['', false], [null, false], @@ -161,11 +163,11 @@ class UtilsTest extends \PHPUnit_Framework_TestCase ['BB', false], ['G', false], ['GG', false], - ['-1', false], - ['-123', false], - ['-1A', false], - ['-1B', false], - ['-123G', false], + ['-1', -1], + ['-123', -123], + ['-1A', -1], + ['-1B', -1], + ['-123G', -123], ['-B', false], ['-A', false], ['-', false], @@ -175,13 +177,13 @@ class UtilsTest extends \PHPUnit_Framework_TestCase } /** - * @dataProvider provideMemoryIniValuesToConvertToBytes + * @dataProvider provideIniValuesToConvertToBytes * @param mixed $input * @param int|false $expected */ - public function testMemoryIniValueToBytes($input, $expected) + public function testExpandIniShorthandBytes($input, $expected) { - $result = Utils::memoryIniValueToBytes($input); + $result = Utils::expandIniShorthandBytes($input); $this->assertEquals($expected, $result); } }