Simplify memoryIniValueToBytes, tweak code to use less memory overall

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;
     }

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);
-
-        if (empty($val)) {
-            return false;
+        // support -1
+        if ((int) $val < 0) {
+            return (int) $val;
         }
 
-        $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) {
+        if (!preg_match('/^\s*(?<val>\d+)(?:\.\d+)?\s*(?<unit>[gmk]?)\s*$/i', $val, $match)) {
             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':

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();

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);
     }
 }