From 118ab680ba9103737b3228d549b1fce4273f9aa8 Mon Sep 17 00:00:00 2001 From: Dejan Markic Date: Thu, 17 Nov 2022 09:45:29 +0100 Subject: [PATCH 01/12] Support protocol packages larger than 16 MiB #47 --- src/Io/Parser.php | 70 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 15 deletions(-) diff --git a/src/Io/Parser.php b/src/Io/Parser.php index 7c07b4a..cafde3c 100644 --- a/src/Io/Parser.php +++ b/src/Io/Parser.php @@ -103,6 +103,10 @@ class Parser * @var Executor */ protected $executor; + /** + * Packet for packet splitting + */ + protected $packet = null; public function __construct(DuplexStreamInterface $stream, Executor $executor) { @@ -154,22 +158,45 @@ public function handleData($data) return; } - $packet = $this->buffer->readBuffer($this->pctSize); + if ($this->packet !== null) { + /** + * We are in packet splitting + * Append data + */ + $packet = null; + $this->packet->append($this->buffer->read($this->pctSize)); + if ($this->pctSize < 0xffffff) { + /** + * We're done + */ + $packet = $this->packet; + $this->packet = null; + } + } else { + $packet = $this->buffer->readBuffer($this->pctSize); + } $this->state = self::STATE_STANDBY; $this->pctSize = self::PACKET_SIZE_HEADER; - try { - $this->parsePacket($packet); - } catch (\UnderflowException $e) { - $this->onError(new \UnexpectedValueException('Unexpected protocol error, received malformed packet: ' . $e->getMessage(), 0, $e)); - $this->stream->close(); - return; - } - - if ($packet->length() !== 0) { - $this->onError(new \UnexpectedValueException('Unexpected protocol error, received malformed packet with ' . $packet->length() . ' unknown byte(s)')); - $this->stream->close(); - return; + if ($this->packet === null && $packet->length() == 0xffffff) { + /** + * Start reading splitted packets + */ + $this->packet = $packet; + } elseif ($packet !== null) { + try { + $this->parsePacket($packet); + } catch (\UnderflowException $e) { + $this->onError(new \UnexpectedValueException('Unexpected protocol error, received malformed packet: ' . $e->getMessage(), 0, $e)); + $this->stream->close(); + return; + } + + if ($packet->length() !== 0) { + $this->onError(new \UnexpectedValueException('Unexpected protocol error, received malformed packet with ' . $packet->length() . ' unknown byte(s)')); + $this->stream->close(); + return; + } } } } @@ -251,7 +278,7 @@ private function parsePacket(Buffer $packet) $this->debug(sprintf("AffectedRows: %d, InsertId: %d, WarningCount:%d", $this->affectedRows, $this->insertId, $this->warningCount)); $this->onSuccess(); $this->nextRequest(); - } elseif ($fieldCount === 0xFE) { + } elseif ($fieldCount === 0xFE && $packet->length() < 0xfffffe) { // EOF Packet $packet->skip(4); // warn, status if ($this->rsState === self::RS_STATE_ROW) { @@ -377,7 +404,20 @@ public function onClose() public function sendPacket($packet) { - return $this->stream->write($this->buffer->buildInt3(\strlen($packet)) . $this->buffer->buildInt1($this->seq++) . $packet); + /** + * If packet is longer than 0xffffff, we should split and send many packets + */ + if (\strlen($packet) > 0xffffff) { + $ret = null; + while (\strlen($packet) > 0) { + $part = substr($packet, 0, 0xffffff); + $ret = $this->stream->write($this->buffer->buildInt3(\strlen($part)) . $this->buffer->buildInt1($this->seq++) . $part); + $packet = substr($packet, strlen($part)); + } + return $ret; + } else { + return $this->stream->write($this->buffer->buildInt3(\strlen($packet)) . $this->buffer->buildInt1($this->seq++) . $packet); + } } protected function nextRequest($isHandshake = false) From 21974a70ede033ab75a9d0ec4e9758169ef36490 Mon Sep 17 00:00:00 2001 From: Dejan Markic Date: Thu, 24 Nov 2022 18:26:38 +0100 Subject: [PATCH 02/12] Fix end of splitted packets. Added tests. Related to: Support protocol packages larger than 16 MiB #47 --- src/Io/Parser.php | 29 +++++++++--- tests/ResultQueryTest.php | 99 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 7 deletions(-) diff --git a/src/Io/Parser.php b/src/Io/Parser.php index cafde3c..b5dc199 100644 --- a/src/Io/Parser.php +++ b/src/Io/Parser.php @@ -175,10 +175,14 @@ public function handleData($data) } else { $packet = $this->buffer->readBuffer($this->pctSize); } + /** + * Remember last packet size as splitted packets may have ended with 0 length packet. + */ + $lastPctSize = $this->pctSize; $this->state = self::STATE_STANDBY; $this->pctSize = self::PACKET_SIZE_HEADER; - if ($this->packet === null && $packet->length() == 0xffffff) { + if ($this->packet === null && $packet->length() == 0xffffff && $lastPctSize > 0) { /** * Start reading splitted packets */ @@ -406,17 +410,28 @@ public function sendPacket($packet) { /** * If packet is longer than 0xffffff, we should split and send many packets + * */ - if (\strlen($packet) > 0xffffff) { + $packet_len = \strlen($packet); + if ($packet_len >= 0xffffff) { $ret = null; - while (\strlen($packet) > 0) { - $part = substr($packet, 0, 0xffffff); - $ret = $this->stream->write($this->buffer->buildInt3(\strlen($part)) . $this->buffer->buildInt1($this->seq++) . $part); - $packet = substr($packet, strlen($part)); + while ($packet_len > 0) { + $part = \substr($packet, 0, 0xffffff); + $part_len = \strlen($part); + $ret = $this->stream->write($this->buffer->buildInt3($part_len) . $this->buffer->buildInt1($this->seq++) . $part); + $packet = \substr($packet, $part_len); + $packet_len = \strlen($packet); + /** + * If last part was exactly 0xffffff in size, we need to send an empty packet to signal end + * of packet splitting. + */ + if (\strlen($packet) == 0 && $part_len == 0xffffff) { + $ret = $this->stream->write($this->buffer->buildInt3(0) . $this->buffer->buildInt1($this->seq++)); + } } return $ret; } else { - return $this->stream->write($this->buffer->buildInt3(\strlen($packet)) . $this->buffer->buildInt1($this->seq++) . $packet); + return $this->stream->write($this->buffer->buildInt3($packet_len) . $this->buffer->buildInt1($this->seq++) . $packet); } } diff --git a/tests/ResultQueryTest.php b/tests/ResultQueryTest.php index 2375a52..4f53aa0 100644 --- a/tests/ResultQueryTest.php +++ b/tests/ResultQueryTest.php @@ -588,4 +588,103 @@ public function testQueryStreamFromLazyConnectionWillErrorWhenConnectionIsClosed $connection->close(); } + + /** + * This should not trigger splitted packets sending + */ + public function testSelectStaticTextSplittedPacketsExactlyBelow16MiB() + { + $connection = $this->createConnection(Loop::get()); + + /** + * This should be exactly below 16MiB packet + * + * x03 + "select ''" = len(10) + */ + $text = str_repeat('A', 0xffffff - 11); + $connection->query('select \'' . $text . '\'')->then(function (QueryResult $command) use ($text) { + $this->assertCount(1, $command->resultRows); + $this->assertCount(1, $command->resultRows[0]); + $this->assertSame($text, reset($command->resultRows[0])); + + $this->assertInstanceOf('React\MySQL\Connection', $conn); + }); + + $connection->quit(); + Loop::run(); + } + + /** + * This should trigger splitted packets sending and + * will send additional empty packet to signal to the server that splitted packets has ended. + */ + public function testSelectStaticTextSplittedPacketsExactly16MiB() + { + $connection = $this->createConnection(Loop::get()); + + /** + * This should be exactly at 16MiB packet + * + * x03 + "select ''" = len(10) + */ + $text = str_repeat('A', 0xffffff - 10); + $connection->query('select \'' . $text . '\'')->then(function (QueryResult $command) use ($text) { + $this->assertCount(1, $command->resultRows); + $this->assertCount(1, $command->resultRows[0]); + $this->assertSame($text, reset($command->resultRows[0])); + + $this->assertInstanceOf('React\MySQL\Connection', $conn); + }); + + $connection->quit(); + Loop::run(); + } + + public function testSelectStaticTextSplittedPacketsAbove16MiB() + { + $connection = $this->createConnection(Loop::get()); + + /** + * This should be exactly at 16MiB + 10 packet + * + * x03 + "select ''" = len(10) + */ + $text = str_repeat('A', 0xffffff); + $connection->query('select \'' . $text . '\'')->then(function (QueryResult $command) use ($text) { + $this->assertCount(1, $command->resultRows); + $this->assertCount(1, $command->resultRows[0]); + $this->assertSame($text, reset($command->resultRows[0])); + + $this->assertInstanceOf('React\MySQL\Connection', $conn); + }); + + $connection->quit(); + Loop::run(); + } + + /** + * Here we force the server to send us an empty packet when splitted packets are to be ended. + */ + public function testSelectStaticTextSplittedPacketsExactly16MiBResponse() + { + $connection = $this->createConnection(Loop::get()); + + /** + * Server response will be exatctly 16MiB, so server will send another empty packet + * to signal end of splitted packets. + * + * x03 + "select ''" = len(10) + */ + $text = str_repeat('A', 0xffffff - 4); + $connection->query('select \'' . $text . '\'')->then(function (QueryResult $command) use ($text) { + $this->assertCount(1, $command->resultRows); + $this->assertCount(1, $command->resultRows[0]); + $this->assertSame($text, reset($command->resultRows[0])); + + $this->assertInstanceOf('React\MySQL\Connection', $conn); + }); + + $connection->quit(); + Loop::run(); + } } From cd6a9a501f167cc577ea15296a46a23393c4835e Mon Sep 17 00:00:00 2001 From: Dejan Markic Date: Mon, 28 Nov 2022 15:57:00 +0100 Subject: [PATCH 03/12] Add ->done() to unused promises returned, so we receive possible exception. Related to Support protocol packages larger than 16 MiB #47 #166 --- tests/ResultQueryTest.php | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/tests/ResultQueryTest.php b/tests/ResultQueryTest.php index 4f53aa0..3adc544 100644 --- a/tests/ResultQueryTest.php +++ b/tests/ResultQueryTest.php @@ -606,9 +606,7 @@ public function testSelectStaticTextSplittedPacketsExactlyBelow16MiB() $this->assertCount(1, $command->resultRows); $this->assertCount(1, $command->resultRows[0]); $this->assertSame($text, reset($command->resultRows[0])); - - $this->assertInstanceOf('React\MySQL\Connection', $conn); - }); + })->done(); $connection->quit(); Loop::run(); @@ -632,9 +630,7 @@ public function testSelectStaticTextSplittedPacketsExactly16MiB() $this->assertCount(1, $command->resultRows); $this->assertCount(1, $command->resultRows[0]); $this->assertSame($text, reset($command->resultRows[0])); - - $this->assertInstanceOf('React\MySQL\Connection', $conn); - }); + })->done(); $connection->quit(); Loop::run(); @@ -654,9 +650,7 @@ public function testSelectStaticTextSplittedPacketsAbove16MiB() $this->assertCount(1, $command->resultRows); $this->assertCount(1, $command->resultRows[0]); $this->assertSame($text, reset($command->resultRows[0])); - - $this->assertInstanceOf('React\MySQL\Connection', $conn); - }); + })->done(); $connection->quit(); Loop::run(); @@ -680,9 +674,7 @@ public function testSelectStaticTextSplittedPacketsExactly16MiBResponse() $this->assertCount(1, $command->resultRows); $this->assertCount(1, $command->resultRows[0]); $this->assertSame($text, reset($command->resultRows[0])); - - $this->assertInstanceOf('React\MySQL\Connection', $conn); - }); + })->done(); $connection->quit(); Loop::run(); From 791bb042060ff01dc35659c840a9bc1eac2ba3ec Mon Sep 17 00:00:00 2001 From: Dejan Markic Date: Fri, 16 Dec 2022 11:57:23 +0100 Subject: [PATCH 04/12] Attempt to increase max_allowed_packet. if not successful mark test as skipped --- tests/ResultQueryTest.php | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/tests/ResultQueryTest.php b/tests/ResultQueryTest.php index 3adc544..3ac780f 100644 --- a/tests/ResultQueryTest.php +++ b/tests/ResultQueryTest.php @@ -592,7 +592,7 @@ public function testQueryStreamFromLazyConnectionWillErrorWhenConnectionIsClosed /** * This should not trigger splitted packets sending */ - public function testSelectStaticTextSplittedPacketsExactlyBelow16MiB() + public function testSelectStaticTextSplitPacketsExactlyBelow16MiB() { $connection = $this->createConnection(Loop::get()); @@ -612,14 +612,34 @@ public function testSelectStaticTextSplittedPacketsExactlyBelow16MiB() Loop::run(); } + protected function checkMaxAllowedPacket($connection): bool + { + $min = 0x1100000; // 17 MiB + $res = \React\Async\await($connection->query('SHOW VARIABLES LIKE \'max_allowed_packet\'')); + $current_max_allowed_packet = $res->resultRows[0]['Value']; + if ($current_max_allowed_packet < $min) { + try { + \React\Async\await($connection->query('SET GLOBAL max_allowed_packet = ?', [0x1100000])); + } catch (\Throwable $e) { + fwrite(STDERR, "checkMaxAllowedPacket: " . $e->getMessage() . "\n"); + } + } + return true; + } + /** * This should trigger splitted packets sending and * will send additional empty packet to signal to the server that splitted packets has ended. */ - public function testSelectStaticTextSplittedPacketsExactly16MiB() + public function testSelectStaticTextSplitPacketsExactly16MiB() { $connection = $this->createConnection(Loop::get()); + if ($this->checkMaxAllowedPacket($connection) === false) { + $this->markTestIncomplete('Cannot test split-packet. max_allowed_packet too low and cannot be adjusted'); + return; + } + /** * This should be exactly at 16MiB packet * @@ -636,10 +656,15 @@ public function testSelectStaticTextSplittedPacketsExactly16MiB() Loop::run(); } - public function testSelectStaticTextSplittedPacketsAbove16MiB() + public function testSelectStaticTextSplitPacketsAbove16MiB() { $connection = $this->createConnection(Loop::get()); + if ($this->checkMaxAllowedPacket($connection) === false) { + $this->markTestIncomplete('Cannot test split-packet. max_allowed_packet too low and cannot be adjusted'); + return; + } + /** * This should be exactly at 16MiB + 10 packet * @@ -659,10 +684,15 @@ public function testSelectStaticTextSplittedPacketsAbove16MiB() /** * Here we force the server to send us an empty packet when splitted packets are to be ended. */ - public function testSelectStaticTextSplittedPacketsExactly16MiBResponse() + public function testSelectStaticTextSplitPacketsExactly16MiBResponse() { $connection = $this->createConnection(Loop::get()); + if ($this->checkMaxAllowedPacket($connection) === false) { + $this->markTestIncomplete('Cannot test split-packet. max_allowed_packet too low and cannot be adjusted'); + return; + } + /** * Server response will be exatctly 16MiB, so server will send another empty packet * to signal end of splitted packets. From 83f87ff546ab306111e78d063475ce03d91cb365 Mon Sep 17 00:00:00 2001 From: Dejan Markic Date: Fri, 16 Dec 2022 12:19:54 +0100 Subject: [PATCH 05/12] Correctly write test and mark skipped where max_allowed_packet cannot be changed --- tests/ResultQueryTest.php | 145 ++++++++++++++++++++++---------------- 1 file changed, 86 insertions(+), 59 deletions(-) diff --git a/tests/ResultQueryTest.php b/tests/ResultQueryTest.php index 3ac780f..02950f3 100644 --- a/tests/ResultQueryTest.php +++ b/tests/ResultQueryTest.php @@ -612,19 +612,22 @@ public function testSelectStaticTextSplitPacketsExactlyBelow16MiB() Loop::run(); } - protected function checkMaxAllowedPacket($connection): bool + protected function checkMaxAllowedPacket($connection): \React\Promise\PromiseInterface { $min = 0x1100000; // 17 MiB - $res = \React\Async\await($connection->query('SHOW VARIABLES LIKE \'max_allowed_packet\'')); - $current_max_allowed_packet = $res->resultRows[0]['Value']; - if ($current_max_allowed_packet < $min) { - try { - \React\Async\await($connection->query('SET GLOBAL max_allowed_packet = ?', [0x1100000])); - } catch (\Throwable $e) { - fwrite(STDERR, "checkMaxAllowedPacket: " . $e->getMessage() . "\n"); + return $connection->query('SHOW VARIABLES LIKE \'max_allowed_packet\'')->then( + function ($res) { + $current_max_allowed_packet = $res->resultRows[0]['Value']; + if ($current_max_allowed_packet < $min) { + return $connection->query('SET GLOBAL max_allowed_packet = ?', [0x1100000]); + } + return \React\Promise\resolve(); } - } - return true; + )->then( + function () { + return true; + } + ); } /** @@ -635,24 +638,32 @@ public function testSelectStaticTextSplitPacketsExactly16MiB() { $connection = $this->createConnection(Loop::get()); - if ($this->checkMaxAllowedPacket($connection) === false) { - $this->markTestIncomplete('Cannot test split-packet. max_allowed_packet too low and cannot be adjusted'); - return; - } + $promise = $this->checkMaxAllowedPacket($connection); - /** - * This should be exactly at 16MiB packet - * - * x03 + "select ''" = len(10) - */ - $text = str_repeat('A', 0xffffff - 10); - $connection->query('select \'' . $text . '\'')->then(function (QueryResult $command) use ($text) { - $this->assertCount(1, $command->resultRows); - $this->assertCount(1, $command->resultRows[0]); - $this->assertSame($text, reset($command->resultRows[0])); - })->done(); + $promise->then( + function () use ($connection) { + /** + * This should be exactly at 16MiB packet + * + * x03 + "select ''" = len(10) + */ + $text = str_repeat('A', 0xffffff - 10); + $connection->query('select \'' . $text . '\'')->then(function (QueryResult $command) use ($text) { + $this->assertCount(1, $command->resultRows); + $this->assertCount(1, $command->resultRows[0]); + $this->assertSame($text, reset($command->resultRows[0])); + })->done(); + } + )->otherwise( + function () { + $this->markTestIncomplete('Could not adjust max_allowed_packet'); + } + )->always( + function () use ($connection) { + $connection->quit(); + } + )->done(); - $connection->quit(); Loop::run(); } @@ -660,24 +671,32 @@ public function testSelectStaticTextSplitPacketsAbove16MiB() { $connection = $this->createConnection(Loop::get()); - if ($this->checkMaxAllowedPacket($connection) === false) { - $this->markTestIncomplete('Cannot test split-packet. max_allowed_packet too low and cannot be adjusted'); - return; - } + $promise = $this->checkMaxAllowedPacket($connection); - /** - * This should be exactly at 16MiB + 10 packet - * - * x03 + "select ''" = len(10) - */ - $text = str_repeat('A', 0xffffff); - $connection->query('select \'' . $text . '\'')->then(function (QueryResult $command) use ($text) { - $this->assertCount(1, $command->resultRows); - $this->assertCount(1, $command->resultRows[0]); - $this->assertSame($text, reset($command->resultRows[0])); - })->done(); + $promise->then( + function () use ($connection) { + /** + * This should be exactly at 16MiB + 10 packet + * + * x03 + "select ''" = len(10) + */ + $text = str_repeat('A', 0xffffff); + $connection->query('select \'' . $text . '\'')->then(function (QueryResult $command) use ($text) { + $this->assertCount(1, $command->resultRows); + $this->assertCount(1, $command->resultRows[0]); + $this->assertSame($text, reset($command->resultRows[0])); + })->done(); + } + )->otherwise( + function () { + $this->markTestIncomplete('Could not adjust max_allowed_packet'); + } + )->always( + function () use ($connection) { + $connection->quit(); + } + )->done(); - $connection->quit(); Loop::run(); } @@ -688,25 +707,33 @@ public function testSelectStaticTextSplitPacketsExactly16MiBResponse() { $connection = $this->createConnection(Loop::get()); - if ($this->checkMaxAllowedPacket($connection) === false) { - $this->markTestIncomplete('Cannot test split-packet. max_allowed_packet too low and cannot be adjusted'); - return; - } + $promise = $this->checkMaxAllowedPacket($connection); - /** - * Server response will be exatctly 16MiB, so server will send another empty packet - * to signal end of splitted packets. - * - * x03 + "select ''" = len(10) - */ - $text = str_repeat('A', 0xffffff - 4); - $connection->query('select \'' . $text . '\'')->then(function (QueryResult $command) use ($text) { - $this->assertCount(1, $command->resultRows); - $this->assertCount(1, $command->resultRows[0]); - $this->assertSame($text, reset($command->resultRows[0])); - })->done(); + $promise->then( + function () use ($connection) { + /** + * Server response will be exatctly 16MiB, so server will send another empty packet + * to signal end of splitted packets. + * + * x03 + "select ''" = len(10) + */ + $text = str_repeat('A', 0xffffff - 4); + $connection->query('select \'' . $text . '\'')->then(function (QueryResult $command) use ($text) { + $this->assertCount(1, $command->resultRows); + $this->assertCount(1, $command->resultRows[0]); + $this->assertSame($text, reset($command->resultRows[0])); + })->done(); + } + )->otherwise( + function () { + $this->markTestIncomplete('Could not adjust max_allowed_packet'); + } + )->always( + function () use ($connection) { + $connection->quit(); + } + )->done(); - $connection->quit(); Loop::run(); } } From f590b099e00e2fa0fd6866d17c52d264dab701af Mon Sep 17 00:00:00 2001 From: Dejan Markic Date: Fri, 16 Dec 2022 12:27:15 +0100 Subject: [PATCH 06/12] Fix some typos. Packet split tests work with correct privileges --- tests/ResultQueryTest.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/ResultQueryTest.php b/tests/ResultQueryTest.php index 02950f3..23734b7 100644 --- a/tests/ResultQueryTest.php +++ b/tests/ResultQueryTest.php @@ -614,12 +614,12 @@ public function testSelectStaticTextSplitPacketsExactlyBelow16MiB() protected function checkMaxAllowedPacket($connection): \React\Promise\PromiseInterface { - $min = 0x1100000; // 17 MiB return $connection->query('SHOW VARIABLES LIKE \'max_allowed_packet\'')->then( - function ($res) { + function ($res) use ($connection) { + $min = 0x1100000; // 17 MiB $current_max_allowed_packet = $res->resultRows[0]['Value']; if ($current_max_allowed_packet < $min) { - return $connection->query('SET GLOBAL max_allowed_packet = ?', [0x1100000]); + return $connection->query('SET GLOBAL max_allowed_packet = ?', [$min]); } return \React\Promise\resolve(); } @@ -655,8 +655,8 @@ function () use ($connection) { })->done(); } )->otherwise( - function () { - $this->markTestIncomplete('Could not adjust max_allowed_packet'); + function (\Throwable $e) { + $this->markTestIncomplete('checkMaxAllowedPacket: ' . $e->getMessage()); } )->always( function () use ($connection) { @@ -688,8 +688,8 @@ function () use ($connection) { })->done(); } )->otherwise( - function () { - $this->markTestIncomplete('Could not adjust max_allowed_packet'); + function (\Throwable $e) { + $this->markTestIncomplete('checkMaxAllowedPacket: ' . $e->getMessage()); } )->always( function () use ($connection) { @@ -725,8 +725,8 @@ function () use ($connection) { })->done(); } )->otherwise( - function () { - $this->markTestIncomplete('Could not adjust max_allowed_packet'); + function (\Throwable $e) { + $this->markTestIncomplete('checkMaxAllowedPacket: ' . $e->getMessage()); } )->always( function () use ($connection) { From bca679c0e9752fab6d94bd120a6e048475b24f32 Mon Sep 17 00:00:00 2001 From: Dejan Markic Date: Fri, 16 Dec 2022 12:36:58 +0100 Subject: [PATCH 07/12] Trigger another action --- src/Io/Parser.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Io/Parser.php b/src/Io/Parser.php index b5dc199..1e9485d 100644 --- a/src/Io/Parser.php +++ b/src/Io/Parser.php @@ -154,7 +154,6 @@ public function handleData($data) $len = $this->buffer->length(); if ($len < $this->pctSize) { $this->debug('Waiting for complete packet with ' . $len . '/' . $this->pctSize . ' bytes'); - return; } @@ -195,7 +194,6 @@ public function handleData($data) $this->stream->close(); return; } - if ($packet->length() !== 0) { $this->onError(new \UnexpectedValueException('Unexpected protocol error, received malformed packet with ' . $packet->length() . ' unknown byte(s)')); $this->stream->close(); From 2262d813f1879468871cef6b6b6c1c13f1e6206c Mon Sep 17 00:00:00 2001 From: Dejan Markic Date: Fri, 16 Dec 2022 12:47:04 +0100 Subject: [PATCH 08/12] If we cannot set max_allowed_packet, report current and desired in error --- tests/ResultQueryTest.php | 77 ++++++++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 29 deletions(-) diff --git a/tests/ResultQueryTest.php b/tests/ResultQueryTest.php index 23734b7..9f67884 100644 --- a/tests/ResultQueryTest.php +++ b/tests/ResultQueryTest.php @@ -589,37 +589,20 @@ public function testQueryStreamFromLazyConnectionWillErrorWhenConnectionIsClosed $connection->close(); } - /** - * This should not trigger splitted packets sending - */ - public function testSelectStaticTextSplitPacketsExactlyBelow16MiB() - { - $connection = $this->createConnection(Loop::get()); - - /** - * This should be exactly below 16MiB packet - * - * x03 + "select ''" = len(10) - */ - $text = str_repeat('A', 0xffffff - 11); - $connection->query('select \'' . $text . '\'')->then(function (QueryResult $command) use ($text) { - $this->assertCount(1, $command->resultRows); - $this->assertCount(1, $command->resultRows[0]); - $this->assertSame($text, reset($command->resultRows[0])); - })->done(); - - $connection->quit(); - Loop::run(); - } - - protected function checkMaxAllowedPacket($connection): \React\Promise\PromiseInterface + protected function checkMaxAllowedPacket($connection, $min = 0x1100000): \React\Promise\PromiseInterface { return $connection->query('SHOW VARIABLES LIKE \'max_allowed_packet\'')->then( - function ($res) use ($connection) { - $min = 0x1100000; // 17 MiB - $current_max_allowed_packet = $res->resultRows[0]['Value']; - if ($current_max_allowed_packet < $min) { - return $connection->query('SET GLOBAL max_allowed_packet = ?', [$min]); + function ($res) use ($min, $connection) { + $current = $res->resultRows[0]['Value']; + if ($current < $min) { + return $connection->query('SET GLOBAL max_allowed_packet = ?', [$min])->otherwise( + function (\Throwable $e) use ($min, $current) { + throw new \Exception( + 'Cannot set max_allowed_packet to: ' . $min . ' ' . + 'current: ' . $current . ' error: ' . $e->getMessage() + ); + } + ); } return \React\Promise\resolve(); } @@ -630,6 +613,42 @@ function () { ); } + /** + * This should not trigger splitted packets sending + */ + public function testSelectStaticTextSplitPacketsExactlyBelow16MiB() + { + $connection = $this->createConnection(Loop::get()); + + $promise = $this->checkMaxAllowedPacket($connection, 0x1000000); // 16MiB + + $promise->then( + function () use ($connection) { + /** + * This should be exactly below 16MiB packet + * + * x03 + "select ''" = len(10) + */ + $text = str_repeat('A', 0xffffff - 11); + $connection->query('select \'' . $text . '\'')->then(function (QueryResult $command) use ($text) { + $this->assertCount(1, $command->resultRows); + $this->assertCount(1, $command->resultRows[0]); + $this->assertSame($text, reset($command->resultRows[0])); + })->done(); + } + )->otherwise( + function (\Throwable $e) { + $this->markTestIncomplete('checkMaxAllowedPacket: ' . $e->getMessage()); + } + )->always( + function () use ($connection) { + $connection->quit(); + } + )->done(); + + Loop::run(); + } + /** * This should trigger splitted packets sending and * will send additional empty packet to signal to the server that splitted packets has ended. From 7514fe9d14cd18198dd0a8963f08f8a287f6c951 Mon Sep 17 00:00:00 2001 From: Dejan Markic Date: Wed, 4 Jan 2023 11:39:05 +0100 Subject: [PATCH 09/12] No return types --- tests/ResultQueryTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ResultQueryTest.php b/tests/ResultQueryTest.php index 9f67884..fc70bab 100644 --- a/tests/ResultQueryTest.php +++ b/tests/ResultQueryTest.php @@ -589,7 +589,7 @@ public function testQueryStreamFromLazyConnectionWillErrorWhenConnectionIsClosed $connection->close(); } - protected function checkMaxAllowedPacket($connection, $min = 0x1100000): \React\Promise\PromiseInterface + protected function checkMaxAllowedPacket($connection, $min = 0x1100000) { return $connection->query('SHOW VARIABLES LIKE \'max_allowed_packet\'')->then( function ($res) use ($min, $connection) { From 0a99c8bbf26cde15e3f2657a2f6f30d4a5cd7cde Mon Sep 17 00:00:00 2001 From: Dejan Markic Date: Wed, 4 Jan 2023 11:39:22 +0100 Subject: [PATCH 10/12] Increase max allowed packet to 17MiB for testing --- .github/workflows/ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6aa1354..a155695 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,7 +29,7 @@ jobs: php-version: ${{ matrix.php }} coverage: xdebug - run: composer install - - run: docker run -d --name mysql --net=host -e MYSQL_RANDOM_ROOT_PASSWORD=yes -e MYSQL_DATABASE=test -e MYSQL_USER=test -e MYSQL_PASSWORD=test mysql:5 + - run: docker run -d --name mysql --net=host -e MYSQL_RANDOM_ROOT_PASSWORD=yes -e MYSQL_DATABASE=test -e MYSQL_USER=test -e MYSQL_PASSWORD=test mysql:5 --max-allowed-packet=17825792 - run: bash tests/wait-for-mysql.sh - run: MYSQL_USER=test MYSQL_PASSWORD=test vendor/bin/phpunit --coverage-text if: ${{ matrix.php >= 7.3 }} @@ -47,6 +47,6 @@ jobs: version: lts-3.30 - run: composer self-update --2.2 # downgrade Composer for HHVM - run: hhvm $(which composer) require phpunit/phpunit:^5 --dev --no-interaction - - run: docker run -d --name mysql --net=host -e MYSQL_RANDOM_ROOT_PASSWORD=yes -e MYSQL_DATABASE=test -e MYSQL_USER=test -e MYSQL_PASSWORD=test mysql:5 + - run: docker run -d --name mysql --net=host -e MYSQL_RANDOM_ROOT_PASSWORD=yes -e MYSQL_DATABASE=test -e MYSQL_USER=test -e MYSQL_PASSWORD=test mysql:5 --max-allowed-packet=17825792 - run: bash tests/wait-for-mysql.sh - run: MYSQL_USER=test MYSQL_PASSWORD=test hhvm vendor/bin/phpunit From 0af7c0beb6cac623c369f989a9fb0af85d3ba62a Mon Sep 17 00:00:00 2001 From: Dejan Markic Date: Wed, 4 Jan 2023 14:56:44 +0100 Subject: [PATCH 11/12] Switch to 20M max_allowed_packet and small code/comments modifications --- .github/workflows/ci.yml | 4 ++-- src/Io/Parser.php | 10 +++++----- tests/ResultQueryTest.php | 8 ++++---- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a155695..e4ae566 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,7 +29,7 @@ jobs: php-version: ${{ matrix.php }} coverage: xdebug - run: composer install - - run: docker run -d --name mysql --net=host -e MYSQL_RANDOM_ROOT_PASSWORD=yes -e MYSQL_DATABASE=test -e MYSQL_USER=test -e MYSQL_PASSWORD=test mysql:5 --max-allowed-packet=17825792 + - run: docker run -d --name mysql --net=host -e MYSQL_RANDOM_ROOT_PASSWORD=yes -e MYSQL_DATABASE=test -e MYSQL_USER=test -e MYSQL_PASSWORD=test mysql:5 --max-allowed-packet=20M - run: bash tests/wait-for-mysql.sh - run: MYSQL_USER=test MYSQL_PASSWORD=test vendor/bin/phpunit --coverage-text if: ${{ matrix.php >= 7.3 }} @@ -47,6 +47,6 @@ jobs: version: lts-3.30 - run: composer self-update --2.2 # downgrade Composer for HHVM - run: hhvm $(which composer) require phpunit/phpunit:^5 --dev --no-interaction - - run: docker run -d --name mysql --net=host -e MYSQL_RANDOM_ROOT_PASSWORD=yes -e MYSQL_DATABASE=test -e MYSQL_USER=test -e MYSQL_PASSWORD=test mysql:5 --max-allowed-packet=17825792 + - run: docker run -d --name mysql --net=host -e MYSQL_RANDOM_ROOT_PASSWORD=yes -e MYSQL_DATABASE=test -e MYSQL_USER=test -e MYSQL_PASSWORD=test mysql:5 --max-allowed-packet=20M - run: bash tests/wait-for-mysql.sh - run: MYSQL_USER=test MYSQL_PASSWORD=test hhvm vendor/bin/phpunit diff --git a/src/Io/Parser.php b/src/Io/Parser.php index 1e9485d..21e68ed 100644 --- a/src/Io/Parser.php +++ b/src/Io/Parser.php @@ -104,7 +104,7 @@ class Parser */ protected $executor; /** - * Packet for packet splitting + * Current packet for split packet paring */ protected $packet = null; @@ -175,15 +175,15 @@ public function handleData($data) $packet = $this->buffer->readBuffer($this->pctSize); } /** - * Remember last packet size as splitted packets may have ended with 0 length packet. + * Remember last packet size as split packets may have ended with 0 length packet. */ $lastPctSize = $this->pctSize; $this->state = self::STATE_STANDBY; $this->pctSize = self::PACKET_SIZE_HEADER; - if ($this->packet === null && $packet->length() == 0xffffff && $lastPctSize > 0) { + if ($this->packet === null && $packet->length() === 0xffffff && $lastPctSize > 0) { /** - * Start reading splitted packets + * Start reading split packets */ $this->packet = $packet; } elseif ($packet !== null) { @@ -423,7 +423,7 @@ public function sendPacket($packet) * If last part was exactly 0xffffff in size, we need to send an empty packet to signal end * of packet splitting. */ - if (\strlen($packet) == 0 && $part_len == 0xffffff) { + if (\strlen($packet) === 0 && $part_len === 0xffffff) { $ret = $this->stream->write($this->buffer->buildInt3(0) . $this->buffer->buildInt1($this->seq++)); } } diff --git a/tests/ResultQueryTest.php b/tests/ResultQueryTest.php index fc70bab..391d9a9 100644 --- a/tests/ResultQueryTest.php +++ b/tests/ResultQueryTest.php @@ -650,8 +650,8 @@ function () use ($connection) { } /** - * This should trigger splitted packets sending and - * will send additional empty packet to signal to the server that splitted packets has ended. + * This should trigger split packets sending and + * will send additional empty packet to signal to the server that split packets has ended. */ public function testSelectStaticTextSplitPacketsExactly16MiB() { @@ -720,7 +720,7 @@ function () use ($connection) { } /** - * Here we force the server to send us an empty packet when splitted packets are to be ended. + * Here we force the server to send us an empty packet when split packets are to be ended. */ public function testSelectStaticTextSplitPacketsExactly16MiBResponse() { @@ -732,7 +732,7 @@ public function testSelectStaticTextSplitPacketsExactly16MiBResponse() function () use ($connection) { /** * Server response will be exatctly 16MiB, so server will send another empty packet - * to signal end of splitted packets. + * to signal end of split packets. * * x03 + "select ''" = len(10) */ From ef50d29c7a2070d06944ee4c8be4c7fe43652ba1 Mon Sep 17 00:00:00 2001 From: Dejan Markic Date: Thu, 19 Jan 2023 12:48:49 +0100 Subject: [PATCH 12/12] Do not set max_allowed_packet, mark test incomplete if max_allowed_packet is too low. Add --max-allowed-packet=20M to README.md docker instructions --- README.md | 2 +- tests/ResultQueryTest.php | 9 +-------- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 6fe2343..77802ca 100644 --- a/README.md +++ b/README.md @@ -551,7 +551,7 @@ For example, to create an empty test database, you can also use a temporary ```bash docker run -it --rm --net=host \ -e MYSQL_RANDOM_ROOT_PASSWORD=yes -e MYSQL_DATABASE=test \ - -e MYSQL_USER=test -e MYSQL_PASSWORD=test mysql:5 + -e MYSQL_USER=test -e MYSQL_PASSWORD=test mysql:5 --max-allowed-packet=20M ``` To run the test suite, go to the project root and run: diff --git a/tests/ResultQueryTest.php b/tests/ResultQueryTest.php index 391d9a9..bb02788 100644 --- a/tests/ResultQueryTest.php +++ b/tests/ResultQueryTest.php @@ -595,14 +595,7 @@ protected function checkMaxAllowedPacket($connection, $min = 0x1100000) function ($res) use ($min, $connection) { $current = $res->resultRows[0]['Value']; if ($current < $min) { - return $connection->query('SET GLOBAL max_allowed_packet = ?', [$min])->otherwise( - function (\Throwable $e) use ($min, $current) { - throw new \Exception( - 'Cannot set max_allowed_packet to: ' . $min . ' ' . - 'current: ' . $current . ' error: ' . $e->getMessage() - ); - } - ); + throw new \Exception('max_allowed_packet too low: current: ' . $current . ' min: ' . $min); } return \React\Promise\resolve(); }