search

friends-of-reactphp / mysql

Async MySQL database client for ReactPHP.
MIT License
331 stars 66 forks source link

Support protocol packages larger than 16 MiB #47 #166

Closed dmarkic closed 3 months ago

dmarkic commented 1 year ago

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

SimonFrings commented 1 year ago

Hey @dmarkic thanks for opening this pull request :+1:

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.

dmarkic commented 1 year ago

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 commented 1 year ago

@dmarkic Thanks for adding tests :+1:

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:

  1. 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.

  2. 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 :+1:

dmarkic commented 1 year ago

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!

dmarkic commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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:

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 :+1:

dmarkic commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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. :+1:

dmarkic commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

dmarkic commented 1 year ago

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

dmarkic commented 1 year ago

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

SimonFrings commented 3 months ago

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

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 commented 3 months ago

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 commented 3 months ago

Alright, then I'll close this one :+1: