Support protocol packages larger than 16 MiB #47 #166
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -588,4 +588,164 @@ public function testQueryStreamFromLazyConnectionWillErrorWhenConnectionIsClosed | |
|
|
||
| $connection->close(); | ||
| } | ||
|
|
||
| protected function checkMaxAllowedPacket($connection, $min = 0x1100000) | ||
| { | ||
| return $connection->query('SHOW VARIABLES LIKE \'max_allowed_packet\'')->then( | ||
| function ($res) use ($min, $connection) { | ||
| $current = $res->resultRows[0]['Value']; | ||
| if ($current < $min) { | ||
| throw new \Exception('max_allowed_packet too low: current: ' . $current . ' min: ' . $min); | ||
| } | ||
| return \React\Promise\resolve(); | ||
| } | ||
| )->then( | ||
| function () { | ||
| return true; | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * 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()); | ||
| } | ||
|
Comment on lines
+633
to
+635
There was a problem hiding this comment. I think we should skip these tests here instead of marking them as incomplete. As the documentation of PHPUnit says to mark tests as incomplete:
The problem here isn't that some implementation is missing, it's more of a "Your configuration doesn't fit our test" case here. This fits the example for skipping tests with PHPUnit as described inside the docs:
Makes more sense, what do you think? |
||
| )->always( | ||
| function () use ($connection) { | ||
| $connection->quit(); | ||
| } | ||
| )->done(); | ||
|
|
||
| Loop::run(); | ||
| } | ||
|
|
||
| /** | ||
| * 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() | ||
| { | ||
| $connection = $this->createConnection(Loop::get()); | ||
|
|
||
| $promise = $this->checkMaxAllowedPacket($connection); | ||
|
|
||
|
Comment on lines
+652
to
+654
There was a problem hiding this comment. I don't think this test behaves the way you intended to. As I see it, this test will be marked as skipped if someone uses a |
||
| $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 (\Throwable $e) { | ||
| $this->markTestIncomplete('checkMaxAllowedPacket: ' . $e->getMessage()); | ||
| } | ||
| )->always( | ||
| function () use ($connection) { | ||
| $connection->quit(); | ||
|
Comment on lines
+669
to
+675
There was a problem hiding this comment. The There was a problem hiding this comment. This only works on react-promise:3.x ... not react-promise:2.7. Will only react-promise:3.x be supported? In current package.json both are supported. There was a problem hiding this comment. You're right that both versions are supported and we already know that v3 will be the way forward. I agree that it should be compatible with both versions at this point, but looking into the future here. If we already know that this functionality will be removed at a later point in time, it makes sense to avoid using these functions, so we don't have to remove them again. There was a problem hiding this comment. Yeah, but 3.x introduced that, 2.7 does not work with |
||
| } | ||
| )->done(); | ||
|
|
||
| Loop::run(); | ||
| } | ||
|
|
||
| public function testSelectStaticTextSplitPacketsAbove16MiB() | ||
| { | ||
| $connection = $this->createConnection(Loop::get()); | ||
|
|
||
| $promise = $this->checkMaxAllowedPacket($connection); | ||
|
|
||
| $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 (\Throwable $e) { | ||
| $this->markTestIncomplete('checkMaxAllowedPacket: ' . $e->getMessage()); | ||
| } | ||
| )->always( | ||
| function () use ($connection) { | ||
| $connection->quit(); | ||
| } | ||
| )->done(); | ||
|
|
||
| Loop::run(); | ||
| } | ||
|
|
||
| /** | ||
| * Here we force the server to send us an empty packet when split packets are to be ended. | ||
| */ | ||
| public function testSelectStaticTextSplitPacketsExactly16MiBResponse() | ||
| { | ||
| $connection = $this->createConnection(Loop::get()); | ||
|
|
||
| $promise = $this->checkMaxAllowedPacket($connection); | ||
|
|
||
| $promise->then( | ||
| function () use ($connection) { | ||
| /** | ||
| * Server response will be exatctly 16MiB, so server will send another empty packet | ||
| * to signal end of split 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 (\Throwable $e) { | ||
| $this->markTestIncomplete('checkMaxAllowedPacket: ' . $e->getMessage()); | ||
| } | ||
| )->always( | ||
| function () use ($connection) { | ||
| $connection->quit(); | ||
| } | ||
| )->done(); | ||
|
|
||
| Loop::run(); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leave this blank line here, improves the readability and keeps the changelog a bit smaller