Support protocol packages larger than 16 MiB #47 #166

dmarkic · 2022-11-17T09:06:12Z

This PR adds support for protocol packages larger than 16MiB, both receiving and sending such packets.

SimonFrings · 2022-11-23T12:25:26Z

Hey @dmarkic thanks for opening this pull request 👍

Can you also add additional unit tests so we can assure that your code works as expected. This is also a great way to test against different edge cases and see if we thought of everything important.

…l packages larger than 16 MiB #47

dmarkic · 2022-11-24T17:33:27Z

Hey @SimonFrings ,
indeed I forgot about both client and server cases, where if last split packet is 0xffffff, client should and server will end split packet sending with an empty packet.
Tests are written in ResultQueryTest as SelectStaticText.
In Mysql max_allowed_packet is by defaultset to 16MB (which is 32MB for client), so maybe the new tests might not pass if max_allowed_packets is not increased when testing.

SimonFrings · 2022-11-28T13:23:15Z

@dmarkic Thanks for adding tests 👍

I took a look at the results from GitHub actions and saw that it shows risky tests. It seems like your assertions are never executed. I think what's happening here is that the query() method you use in your tests throws an exception. If an exception is thrown inside a promise, you can only see it if you explicitly catch it or define a behavior if the promise gets rejected, for example:

$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);
},
    function (Exception $error) {
        echo 'Error: ' . $error->getMessage() . PHP_EOL;
    }
);

You can try this out and look for yourself if any exception is thrown, but I don't recommend adding this to your tests and then commit it to this PR. This way we stay consistent with the other tests in the ResultQueryTest class, but you can still use for debugging purposes.

I know the current behavior when it comes to exception handling is confusing, but we're going to change that with the upcoming v3 for ReactPHP, so that all errors should be shown by default unless explicitly hidden. For more information about our plans for ReactPHP v3, take a look at our v3 announcement.

I really like that you added tests here, in particular you tested the functional side which is totally fine in itself and the right thing to do, but you also have to take a look at the unit side. Can be kind of confusing that we have to types of tests, so I'll quickly break it down:

We use functional tests, like you added, to test the overall functionality of the project. With functional tests we don't have to go much into detail, we only have to show the right behavior of the project in each case. In your case this means that you don't have to use the exact limit as a package size, you just have to show that you can use packages larger than 16 MiB (I'd recommend something like 20 MB). The 16 MiB limit is more of a implementation detail and has to be covered by the unit logic. This way we keep the functional tests easier to understand.
The unit side of our tests goes more into detail. You changed some logic inside the Parser class which means that you have to add new unit tests to the Io/ParserTest class. In here you test against the specific lines of code you added, to assure that every line of code works as expected. You can check the code coverage output from GitHub actions to see how many lines of code in the Parser class are covered. Before your changes we had Lines: 98.12% (157/160) covered, with you changes we now have Lines: 89.56% (163/182). You don't have to achieve 100% here, there were already 3 lines of code not covered, so I would expect the result to land on (179/182) lines.

I'm sorry for the big wall of text here, I tried to give you as much information as possible to help you keep going. I'm happy to help more if you have any further questions 👍

dmarkic · 2022-11-28T14:55:23Z

Thank you for your thorough explanation.

Regarding the failed testSelectStaticTextSplittedPacketsExactly16MiBResponse test. Indeed, error is not caught. I've added ->done() at the end and removed max_allowed_packet setting in my testing MariaDb (so default is used) and I receive this error: React\MySQL\Exception: Got a packet bigger than 'max_allowed_packet' bytes. (as I have predicted in my previous comment) And that's why Parser class is left with untested code, since this test is testing packet that comes from server in packet split mode (in Parser::handleData).

I can write additional Parser tests that would test split packet functionality. I was first going with this functional part first, which is where the real server is tested against the code.

Is there a way to increase max_allowed_packet to maybe 17MB on testing MySql/MariaDb server so functional tests would pass? It's also why I didn't write functional tests that would use packets with 20MB size, etc. Just enough so the protocol it self is tested with the real server. I can do larger packet tests in ParserTest.php directly.

I would suggest however, to change the tests in a way like testSelectStaticTextSplittedPacketsExactly16MiBResponse is now, where, if returned promise is not used, it should be ended with ->done(). That way at least the inner exception is received by unittest.

I've updated the tests so the testSelectStaticTextSplittedPacketsExactly16MiBResponse will fail with correct (if max_allowed_packet is not increased) exception. If max_allowed_packet cannot be set in testing environment, I'll remove that test and write test in ParserTest.php.

Let me know your thoughts. I'll push the changes in few minutes, so we can see the exception.

Thanks!

…ption. Related to Support protocol packages larger than 16 MiB #47 #166

dmarkic · 2022-11-28T15:01:27Z

It seems that all tests fail because of max_allowed_packet. Don't know what's the current setting. On my default installation only the last one fails (since client would receive double the max_allowed_packet).

SimonFrings · 2022-12-07T13:09:04Z

@dmarkic So sounds like max_allowed_packet needs to be increased on the server side, so it depends on your configuration, or am I wrong here?

dmarkic · 2022-12-07T14:29:21Z

@SimonFrings , for tests to work, the server on which tests are performed, needs to have max_allowed_packet increased.

See here: https://github.com/friends-of-reactphp/mysql/actions/runs/3565935698/jobs/5991738760#step:7:62

In this case, the mysql docker server needs max_allowed_packets to 17MB. Otherwise this functional test is not possible (as server will not allow large packets, thus not using packet-split).

If max_allowed_packets cannot be increased on docker mysql, I'll remove those tests and just test the Parser.php. Although I would recomment testing packet-split also against real mysql server.

Thanks!

SimonFrings · 2022-12-08T08:46:19Z

@dmarkic So I guess you're using the docker example from our README.md, right? If so, I quickly eyeballed the mySQL docker image and it seems like you can change configuration by using a custom configuration file (there's also a way to change configuration without a config file): https://hub.docker.com/_/mysql/

Another thing I want to mention here is that we can test this on a unit level so this project behaves the right way, but this doesn't mean that every server will accept these packet sizes and that's why this is "difficult" to test the functional side.

For example, if you test the functional side on your local machine with a changed server configuration, it is likely that all tests will pass, but on GitHub actions the same tests will fail, because we never adjusted the configuration there. One answer could be that we just raise the max_allowed_packets there, but I'm don't think this is something we want to recommend the user to do. This also includes a bunch of new questions like:

What would be an optimal size?
Do we change the currently used sever setting or do we use two servers (one allowing small packages and one allowing big ones) and if so, do we now have to test both across all PHP versions?
...

Another way could be to write a test that will check the max_allowed_packets size on the server side and will just skip the test if the server doesn't allow bigger packages. We'll still have to test this on the unit level and the big plus I see here is, we don't have to adjust the used server configuration for our test matrix. The downside is that we didn't really test the case, but I'm not sure how big of a problem this really is.

If you ask me, I would always suggest to split your big packet into smaller ones if possible, maybe this could also be an answer to your problem. I don't know if there are cases where it is impossible to use smaller packets. If there are it is a good idea that this project support bigger packages.But like I said above, how do we test this the right way?

A lot of questions, I'm interested in your opinion on this 👍

dmarkic · 2022-12-08T10:33:44Z

The only question here is: can/will you adjust the max_allowed_packets on Github actions. It's only about those tests now.

Sure, if someone would want to run tests locally, we can skip tests that would need max_allowed_packets > 16MB.

To answer your questions:

What would be an optimal size?
- For tests 17MB is totally enough
Do we change the currently used sever setting or do we use two servers (one allowing small packages and one allowing big ones) and if so, do we now have to test both across all PHP versions?
- Just change the current servers configuration. There is no other test that would interfere with max_allowed_packets.

Regarding the split of big packets - it's just not feasible (it is possible though). If you have a use case, where something larger than 16MB is stored in MySQL, you will need support for MySQL split packet functionality to fetch/update it without changing your application logic.
Also, what's rather bad about current implementation is, it silently discards such packet. You think you've written something to MySQL, but you didn't. It takes some time to figure out that this is not supported by this package.

If this packet_splitting merges, I'd love to contribute also:

SimonFrings · 2022-12-08T11:43:27Z

Or is it an option to raise the server configuration inside this functional test only, so we don't have to change the whole configuration?

dmarkic · 2022-12-13T10:31:26Z

Hello @SimonFrings ,

Yes, that's a great idea. We could use SET GLOBAL max_allowed_packet = ... to set the max_allowed_packet to a different value. But the user running this query should have SUPER privilege. Don't know if that's the case on Github actions.

It is also possible to skip those tests if current max_allowed_packet is too low. I am in favor of testing this with increasing max_allowed_packet. The easiest is for sure to start mysqld with max-allowed-packet=17M argument.

Let me know what you think.

SimonFrings · 2022-12-14T09:07:39Z

Yes, that's a great idea. We could use SET GLOBAL max_allowed_packet = ... to set the max_allowed_packet to a different value. But the user running this query should have SUPER privilege. Don't know if that's the case on Github actions.

Maybe let's try this first, seems to be the best solution so far, we'll see if it works out. If not we can always come back to our other suggestions. 👍

…s skipped

… be changed

dmarkic · 2022-12-16T11:55:25Z

Hello @SimonFrings ,

Test is now successful, but packet split is not tested as it does not have permission to increase the max_allowed_packet.

To increase max_allowed_packet, MySQL docker would have to be started with --max-allowed-packet=17825792 at the end.

Example from ci.yml line 32:

- 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

Don't know if I can change the ci.yml file to test?

SimonFrings · 2023-01-02T08:00:53Z

@dmarkic Seems a bit random as it would increase max_allowed_packet for every test and not only for the ones you need. Is there no way we can increase max_allowed_packet from the outside?

dmarkic · 2023-01-02T11:33:31Z

@SimonFrings , not really. I tried, but user does not have permission to change max_allowed_packet. It really doesn't change anything for the other tests, except it enables us to test packet split functionality. There is no other way of testing this against real server except with this small change.

SimonFrings · 2023-01-03T06:44:20Z

@dmarkic ok, to keep this going let's change the server settings for now to make this work, maybe we can find a better solution afterwards.

dmarkic · 2023-01-04T10:41:36Z

@SimonFrings , I guess it's all working now with this small change.

SimonFrings · 2023-01-04T13:25:05Z

.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


Seems like a good idea, but I think it makes more sense to use something like this:

Suggested change

- 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

Using 17825792 could seem a bit random for others ^^

Sure, I'll make this change

SimonFrings · 2023-01-04T13:27:42Z

src/Io/Parser.php

+                 * 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) {


Would it make sense to use === instead of == here?

I'll review my code and make changes

dmarkic · 2023-01-10T08:47:54Z

@SimonFrings , I guess that solves the whole PR with packet split support?

SimonFrings · 2023-01-11T07:36:25Z

tests/ResultQueryTest.php

+    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) {
+                    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();
+            }
+        )->then(
+            function () {
+                return true;
+            }
+        );
+    }


You said in one of your last comments:

I tried, but user does not have permission to change max_allowed_packet

In here you're trying to overwrite max_allowed_packet, so I'm a little confused if it works or not. Or maybe this is a leftover? It seems like your tests will never overwrite max_allowed_packet in this case because $min never surpasses $current, right?

Yes, it will try. Maybe useful for someone with smaller max_allowed_packet but permission to change it. If it does not succeed it will mark test as incomplete.

I agree that this is important for someone who wants to execute the tests and has a smaller max_allowed_packet. We also recommend a temporary mysql docker image to execute tests inside our documentation which doesn't involve raising max_allowed_packet, so it will use the standard 16MiB :

For example, to create an empty test database, you can also use a temporary mysql Docker image like this:

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

Over all I am not sure if we should even be able to SET GLOBAL max_allowed_packet as it changes the configuration of a database, which could be used by more than just one person. Changing someone else's gloabl configuration just feels wrong and I can imagine why it shouldn't be allowed. If we want to alter the max_allowed_packet, we should do this temporary on the connection if possible.

As an alternative we could also update our temporary Docker image inside the Readme.md like you already did inside the ci.yml (same docker images btw) and if someone would use a database with a smaller max_allowed_packet we should skip the necessary tests (not mark them incomplete)

What do you think?

Yeah, I agree, we should probably not set GLOBAL configuration. I'll rewrite tests to simply check if max_allowed_packet is set to large enough value for tests, otherwise tests will be marked as incomplete.
I will update the Readme.md file to include max_allowed_packet setting.

…cket is too low. Add --max-allowed-packet=20M to README.md docker instructions

dmarkic · 2023-01-19T11:51:52Z

Sorry for late response. I've fixed the discussed issues.

packet split test are marked as incomplete if max_allowed_packet is too low
README.md docker arguments were updated

SimonFrings

Thanks for updating the PR, we're getting closer and closer 👍

I added a few remarks below, interested what you think about them.

SimonFrings · 2023-01-20T11:12:14Z

src/Io/Parser.php

                $this->debug('Waiting for complete packet with ' . $len . '/' . $this->pctSize . ' bytes');
-
                return;


I think we should leave this blank line here, improves the readability and keeps the changelog a bit smaller

SimonFrings · 2023-01-20T11:21:21Z

tests/ResultQueryTest.php

+            function (\Throwable $e) {
+                $this->markTestIncomplete('checkMaxAllowedPacket: ' . $e->getMessage());
+            }


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:

PHPUnit\Framework\IncompleteTest is a marker interface for marking an exception that is raised by a test method as the result of the test being incomplete or currently not implemented.

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:

In the test case class’ setUp() template method we check whether the MySQLi extension is available and use the markTestSkipped() method to skip the test if it is not.

Makes more sense, what do you think?

SimonFrings · 2023-01-20T11:35:00Z

tests/ResultQueryTest.php

+
+        $promise = $this->checkMaxAllowedPacket($connection);
+


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 max_allowed_packet size of 16 MiB which should be allowed in this case. This is caused by the $min default of your checkMaxAllowedPacket() function, as it will set the minimum to 17MiB and throws an exception for configurations with 16 MiB.

SimonFrings · 2023-01-20T11:35:04Z

tests/ResultQueryTest.php

+        )->otherwise(
+            function (\Throwable $e) {
+                $this->markTestIncomplete('checkMaxAllowedPacket: ' . $e->getMessage());
+            }
+        )->always(
+            function () use ($connection) {
+                $connection->quit();


The PromiseInterface::always() and PromiseInterface::otherwise() functions are marked as deprecated as described in reactphp/promise, I don't think it's a good idea to use them in this context as we want to avoid using deprecated functions.

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.

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.

Yeah, but 3.x introduced that, 2.7 does not work with catch and finally. So, when 2.7 version is removed from this package, it will be time to also change those methods.

SimonFrings · 2024-04-17T10:56:57Z

Hey @dmarkic, this PR is open for a long time now and I wanted to see how it's going 👍

I still think this is a valuable addition to the project, so what can we do to get this to a mergable state. A lot has happened since February 2023 and the first thing to be done here is to rebase and resolve the latest conflicts.

Also taken from my last response, there are still some changes that should be done in here (e.g. avoid using the deprecated otherwise() and always() functions as they were removed in Promised v3).

I want to avoid having pull requests and issues open for too long, so it's also okay to close this if there's currently no time to work on this topic and revisit this at a later point again.

Interested in your thoughts!

dmarkic · 2024-04-17T12:27:14Z

Hello @SimonFrings,

I'll create new PR for 0.7.x version, since a lot has changed. You can close this one and I'll create a new one also with promise v3 support.

I'll do this within few days.

SimonFrings · 2024-04-17T12:33:34Z

Alright, then I'll close this one 👍

Support protocol packages larger than 16 MiB #47

118ab68

Fix end of splitted packets. Added tests. Related to: Support protoco…

21974a7

…l packages larger than 16 MiB #47

Add ->done() to unused promises returned, so we receive possible exce…

cd6a9a5

…ption. Related to Support protocol packages larger than 16 MiB #47 #166

Dejan Markic added 5 commits December 16, 2022 11:57

Attempt to increase max_allowed_packet. if not successful mark test a…

791bb04

…s skipped

Correctly write test and mark skipped where max_allowed_packet cannot…

83f87ff

… be changed

Fix some typos. Packet split tests work with correct privileges

f590b09

Trigger another action

bca679c

If we cannot set max_allowed_packet, report current and desired in error

2262d81

Dejan Markic added 2 commits January 4, 2023 11:39

No return types

7514fe9

Increase max allowed packet to 17MiB for testing

0a99c8b

SimonFrings reviewed Jan 4, 2023

View reviewed changes

Switch to 20M max_allowed_packet and small code/comments modifications

0af7c0b

SimonFrings reviewed Jan 11, 2023

View reviewed changes

Do not set max_allowed_packet, mark test incomplete if max_allowed_pa…

ef50d29

…cket is too low. Add --max-allowed-packet=20M to README.md docker instructions

SimonFrings reviewed Jan 20, 2023

View reviewed changes

SimonFrings closed this Apr 17, 2024

dmarkic mentioned this pull request Apr 18, 2024

Support protocol packages larger than 16 MiB #47 #197

Open

	- 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

		$this->debug('Waiting for complete packet with ' . $len . '/' . $this->pctSize . ' bytes');

		return;

Support protocol packages larger than 16 MiB #47 #166

Support protocol packages larger than 16 MiB #47 #166

Conversation

dmarkic commented Nov 17, 2022

SimonFrings commented Nov 23, 2022

dmarkic commented Nov 24, 2022

SimonFrings commented Nov 28, 2022

dmarkic commented Nov 28, 2022

dmarkic commented Nov 28, 2022

SimonFrings commented Dec 7, 2022

dmarkic commented Dec 7, 2022

SimonFrings commented Dec 8, 2022

dmarkic commented Dec 8, 2022

SimonFrings commented Dec 8, 2022

dmarkic commented Dec 13, 2022

SimonFrings commented Dec 14, 2022

dmarkic commented Dec 16, 2022

SimonFrings commented Jan 2, 2023

dmarkic commented Jan 2, 2023

SimonFrings commented Jan 3, 2023

dmarkic commented Jan 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarkic commented Jan 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarkic commented Jan 19, 2023

SimonFrings left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonFrings commented Apr 17, 2024

dmarkic commented Apr 17, 2024

SimonFrings commented Apr 17, 2024