mysqlnd: support ER_CLIENT_INTERACTION_TIMEOUT #13618

Appla · 2024-03-07T07:05:56Z

MySQL introduced a new error type for wait_timeout in (mysql/mysql-server@14508bb )
which causes mysqlnd report warning Packets out of order. Expected 1 received 0. Packet size=145 see (https://bugs.php.net/bug.php?id=81335)

This MR supports this new error type in a conservative way (and only handles this type of error).

kamil-tekiela · 2024-03-07T17:25:40Z

Could you provide a unit test for this?

If I remember correctly there was a bug report about this. Do you know the number?

ext/mysqlnd/mysqlnd_wireprotocol.c

kamil-tekiela · 2024-03-07T17:33:45Z

ext/mysqlnd/mysqlnd_wireprotocol.c

@@ -292,9 +310,12 @@ mysqlnd_read_packet_header_and_body(MYSQLND_PACKET_HEADER * packet_header,
 {
 	DBG_ENTER("mysqlnd_read_packet_header_and_body");
 	DBG_INF_FMT("buf=%p size=%zu", buf, buf_size);
+	assert(error_info->error_no == 0);


What is this for? Is this necessary?

The following part required error_info->error_no are 0 here, is this always true in here?

To be honest, I don't know if this is guaranteed, but it seems to me like it should be. Either way, I don't think adding an assert is correct here. Can you remove it and see if it still works?

Removing this assert has no effect. I originally considered keeping it in debug mode to catch the impact of other changes in time.

I also think this assert is unnecessary.

Oh, wait a minute. The problem doesn't seem to be here.
It seems like the JIT test is failing, so I'll do some research.

MYSQLND_ERROR_INFO * error_info can be shared when using persistent connections.
In other words, depending on the processing timing of the processes running in parallel, there is a slight possibility that a value other than 0 may be entered here.

I don't know why such a situation occurred in CI testing.

However, it does not reproduce locally even if I run it in parallel. If it's thread-safe, this probably won't happen...

I found that there are routes such as mysqlnd_handle_local_infile that call mysqlnd_read_packet_header_and_body with an error set. Therefore, assert should not be done.

(I'm probably wrong about parallel execution and the value after closing. Please forget it.)

FYI

php-src/ext/mysqlnd/mysqlnd_loaddata.c

Line 163 in edbef3e

SET_CLIENT_ERROR(conn->error_info, CR_LOAD_DATA_LOCAL_INFILE_REJECTED, UNKNOWN_SQLSTATE,

php-src/ext/mysqlnd/mysqlnd_loaddata.c

Line 252 in edbef3e

if (FAIL == conn->payload_decoder_factory->m.send_command_handle_response(

php-src/ext/mysqlnd/mysqlnd_wireprotocol.c

Line 786 in edbef3e

php_mysqlnd_ok_read(MYSQLND_CONN_DATA * conn, void * _packet)

(PROT_OK_PACKET)

php-src/ext/mysqlnd/mysqlnd_wireprotocol.c

Line 802 in edbef3e

if (FAIL == mysqlnd_read_packet_header_and_body(&(packet->header), pfc, vio, stats, error_info, connection_state, buf, buf_len, "OK", PROT_OK_PACKET)) {

Thanks for the deeply analysis.

ext/mysqlnd/mysqlnd_enum_n_def.h

ext/mysqlnd/mysqlnd_wireprotocol.c

Appla · 2024-03-08T01:52:12Z

Could you provide a unit test for this?
I will add a test

If I remember correctly there was a bug report about this. Do you know the number?

Is this one? 81335

kamil-tekiela · 2024-03-08T13:41:39Z

ext/mysqli/tests/bug81335.phpt

+<?php
+if (getenv("SKIP_SLOW_TESTS")) die("skip slow test");
+
+if (!defined('MYSQLI_STORE_RESULT_COPY_DATA')) die('skip requires mysqlnd');


You are making this change against master branch, so mysqlnd is the only option available. This check is redundant and will possibly break in PHP 9.0 once the constant is removed.

The CI jobs are failing. Could you please check why?

Should I make this MR for 8.3?

I will check the reason for the CI failure on windows later.

I think this could even go onto 8.2.

OK, I will update later.

The CI jobs are failing. Could you please check why?

This looks like windows or mysql-server for handle half-close TCP connection differently.

---- update
When a client-timeout occurs, the MySQL server sends an error 4021 and a TCP-FIN packet, and the TCP state changes to FIN_WAIT_2. At this time, the TCP state of the client is CLOSE_WAIT.

MySQL-Server side: when the server TCP state is FIN_WAIT_2, the server will send a TCP RST packet after client sending a query.
If the client is on Linux, it can still read the error message packet sent by the server.
If the client is on Windows, an error (10053) will occur when executing receive.

I'm not sure if this is due to the TCP implementation or something else.

I think this could even go onto 8.2.

The MR target 8.2 now.

kamil-tekiela · 2024-03-08T13:42:14Z

ext/mysqli/tests/bug81335.phpt

+    echo "\n";
+    echo $e->getCode();
+}
+$mysqli->close();


This line is not part of the test, right? Can you remove it please?

This line is not part of the test, right? Can you remove it please?

$mysqli->close();
Is this one?

Yes, you can remove it.

Got it (●'◡'●)

SakiTakamachi · 2024-03-09T12:30:05Z

ext/mysqli/tests/bug81335.phpt

+
+require_once 'connect.inc';
+if (!$link = @my_mysqli_connect($host, $user, $passwd, $db, $port, $socket)) die("skip cannot connect");
+if (mysqli_get_server_version($link) < 80024) {


This is an error message that does not occur with MariaDB, so please skip it if using MariaDB.

Suggested change

if (mysqli_get_server_version($link) < 80024) {

if (mysqli_get_server_version($link) < 80024 || str_contains(mysqli_get_server_info($link), 'MariaDB')) {

SakiTakamachi · 2024-03-09T13:02:07Z

ext/mysqlnd/mysqlnd_wireprotocol.c

+		if (error_info->error_no == 0) {
+			SET_CLIENT_ERROR(error_info, CR_SERVER_GONE_ERROR, UNKNOWN_SQLSTATE, mysqlnd_server_gone);
+		}


For the same reason, I don't think this check should be done either, but is there any reason why error_no has to be 0?

I consider 0 to mean that no error occurred, but this cannot be guaranteed. so may be compare with CLIENT_INTERACTION_TIMEOUT here?

I tried various things.
There may already be a meaningful error set here, so checking if it's 0 makes sense to me.

ext/mysqli/tests/bug81335.phpt

mbeccati · 2024-04-11T13:03:05Z

The fix is currently breaking several DBAL tests. I don't think it is safe for 8.2 and 8.3 and/or it is not working properly.

See: https://github.com/mbeccati/php-latest-builds/runs/23707742797

bukka · 2024-04-11T13:28:28Z

Agreed I think we need to revert this

kamil-tekiela · 2024-04-11T13:44:04Z

Sure, we can revert it, but the test failures seem to be because DBAL is not handling error code 4031

https://github.com/doctrine/dbal/blob/c70bae9e69e0e543044d5e282a624d9b3cbea6fb/src/Driver/API/MySQL/ExceptionConverter.php#L34

I am pretty sure that they need to update their code. Am I missing something here?

bukka · 2024-04-11T13:47:17Z

Yeah looking at it, it seems like it needs to be updated. It has got some specific expectation in some tests like that connection lost is triggered on connection timeout (e.g. ConnectionLostTest expecting ConnectionLost on timeout). The way how I see it is that this is essentially a breaking fix and as such it can happen only in master and it should be propertly noted in UPGRADING.

mbeccati · 2024-04-11T13:50:58Z

Not a DBAL deveoper, but I agree with @bukka that this kind of behaviour change can only happen on master.

However, it's not just the ConnectionLost test that fails, so there might be other edge cases that need to be looked at.

bukka · 2024-04-11T13:53:33Z

Yeah I think it should be reverted and then properly tested with DBAL and make sure that each failure is expected and also ideally document what needs to be done in DBAL to address all failures (even more ideally provide a PR for them).

bukka · 2024-04-11T13:55:36Z

@kamil-tekiela are you happy to revert it or do you want me to do that?

kamil-tekiela · 2024-04-11T14:00:45Z

@bukka Can you please do it? Thanks.

alexdowad · 2024-04-15T15:07:46Z

Hmm, I'm wondering why my review was requested on this PR.

iluuu1994 · 2024-04-15T15:17:33Z

@alexdowad It's because of a rebase before adjusting the target branch.

Appla · 2024-04-22T02:38:25Z

Yeah looking at it, it seems like it needs to be updated. It has got some specific expectation in some tests like that connection lost is triggered on connection timeout (e.g. ConnectionLostTest expecting ConnectionLost on timeout). The way how I see it is that this is essentially a breaking fix and as such it can happen only in master and it should be propertly noted in UPGRADING.

The merge target is master now. Anything else?

mbeccati · 2024-04-22T07:52:36Z

@Appla I can try to give some guidance if needed

mbeccati · 2024-04-22T08:50:51Z

I've tested DBAL (both mysqli and pdo_mysql) with your branch and once 4031 is properly handled, all failures go away - it was simply a matter of consecutive tests not being happy to start without a functioning connection
There should be a mention in the UPGRADING file that mysqlnd now could also raise error 4031 when the connection is lost due to timeouts.
I can take care of making a PR do DBAL so that they are aware of the incoming change

Perhaps other projects could benefit from having better visibility about this. Not sure how best to proceed with that.

It has been introduced in: mysql/mysql-server@14508bb And PHP 8.4 will support it: php/php-src#13618

Appla · 2024-04-24T02:14:38Z

I've tested DBAL (both mysqli and pdo_mysql) with your branch and once 4031 is properly handled, all failures go away - it was simply a matter of consecutive tests not being happy to start without a functioning connection

There should be a mention in the UPGRADING file that mysqlnd now could also raise error 4031 when the connection is lost due to timeouts.

I can take care of making a PR do DBAL so that they are aware of the incoming change

Perhaps other projects could benefit from having better visibility about this. Not sure how best to proceed with that.

Thanks

Appla · 2024-06-17T09:27:57Z

Yeah looking at it, it seems like it needs to be updated. It has got some specific expectation in some tests like that connection lost is triggered on connection timeout (e.g. ConnectionLostTest expecting ConnectionLost on timeout). The way how I see it is that this is essentially a breaking fix and as such it can happen only in master and it should be propertly noted in UPGRADING.

Any updates?

| Q | A |------------- | ----------- | Type | improvement | Fixed issues | Prevents future issues ;-) #### Summary PHP 8.4 will support a new error code 4031 in mysqlnd when the connection is dropped due to timeouts. It has been introduced in: mysql/mysql-server@14508bb And PHP 8.4 will support it: php/php-src#13618 The PR gets the test suite green again (mysqli + pdo_mysql). I have used 4.0.x as base, but feel free to change according to your preferences.

mbeccati

@bukka DBAL has applied the change. Anything else preventing us from merging this?

mconkin · 2024-08-07T01:03:16Z

Correct me if I'm wrong, but isn't there an underlying problem within this bug that eventually propagates into num_affected_rows misreporting?

There was a merge request for it around 10-8 years ago, I think someone named ma11oc? IDK, I read it in passing and they just closed it and deleted their account.

Pretty unprofessional, if I do say so myself.

I digress. This looks incredibly close to the answer. However, by just glancing at this thread it seems like something's off. Might want to recheck ->mysqli_num_affected_rows and see if the root cause is there...

Sincerest apologies for jumping into this so close to release.

I don't have much PHP or C experience (as you can see my account is quite new (and I'm more familiar with Python for web develop so I don't know how useful or necessary this comment is ))

Anyways, to risc embarrassing myself further, quite similar to ma11oc, the original discoverer of this alleged bug, I'll finish with a final statement.

mbeccati · 2024-08-07T08:45:42Z

@mconkin do you have any reference? To the best of my knowledge this PR is meant to support new functionality introduced in recent MySQL versions.

mconkin · 2024-08-07T12:19:19Z

@Appla sure, give me a day or two. Currently limited to a 2017 MacBook Pro. Xcode tools won't install and I refuse to use Homebrew. Let me provide some code lines once I download the zip.

mbeccati · 2024-08-14T08:27:14Z

@SakiTakamachi do you think we can push this a little bit and merge it? I have championed for this and the required changes have been implemented in DBAL. It would be nice to keep the promise and have it in 8.4.

Cheers

bukka · 2024-08-18T14:55:31Z

I think it would make sense for PHP 8.4. Pinging @SakiTakamachi for approval.

SakiTakamachi

To me, that makes sense :)

SakiTakamachi · 2024-08-19T00:13:00Z

I'll merge this tonight if no one else has any concerns

github-actions bot added the Extension: mysqlnd label Mar 7, 2024

Girgias requested review from kamil-tekiela and SakiTakamachi March 7, 2024 14:26

kamil-tekiela reviewed Mar 7, 2024

View reviewed changes

github-actions bot added the Extension: mysqli label Mar 8, 2024

kamil-tekiela reviewed Mar 8, 2024

View reviewed changes

SakiTakamachi reviewed Mar 9, 2024

View reviewed changes

Appla force-pushed the mysqlnd_support_ER_CLIENT_INTERACTION_TIMEOUT branch from 67ef59b to c4ee2ee Compare March 11, 2024 02:19

Appla changed the base branch from master to PHP-8.2 March 11, 2024 02:20

kamil-tekiela reviewed Mar 11, 2024

View reviewed changes

ext/mysqli/tests/bug81335.phpt Outdated Show resolved Hide resolved

kamil-tekiela reviewed Mar 12, 2024

View reviewed changes

ext/mysqli/tests/bug81335.phpt Outdated Show resolved Hide resolved

Appla force-pushed the mysqlnd_support_ER_CLIENT_INTERACTION_TIMEOUT branch from cae3483 to c3fa03b Compare April 10, 2024 08:18

github-actions bot removed the Extension: mysqli label Apr 10, 2024

Appla force-pushed the mysqlnd_support_ER_CLIENT_INTERACTION_TIMEOUT branch from c3fa03b to 92eccc3 Compare April 10, 2024 08:19

github-actions bot added the Extension: mysqli label Apr 10, 2024

kamil-tekiela approved these changes Apr 10, 2024

View reviewed changes

kamil-tekiela closed this in 5035b85 Apr 10, 2024

mbeccati mentioned this pull request Apr 22, 2024

Properly handle MySQL error code 4031 from PHP 8.4 doctrine/dbal#6363

Merged

mbeccati added a commit to mbeccati/dbal that referenced this pull request Apr 22, 2024

Properly handle MySQL error code 4031 from PHP 8.4

94d72ea

It has been introduced in: mysql/mysql-server@14508bb And PHP 8.4 will support it: php/php-src#13618

mbeccati added a commit to mbeccati/dbal that referenced this pull request Apr 22, 2024

Properly handle MySQL error code 4031 from PHP 8.4

d0b5e1a

It has been introduced in: mysql/mysql-server@14508bb And PHP 8.4 will support it: php/php-src#13618

github-actions bot added Extension: mysqli Extension: mysqlnd labels Apr 24, 2024

Appla requested a review from bukka May 10, 2024 12:06

mbeccati self-requested a review July 23, 2024 21:29

mbeccati approved these changes Jul 23, 2024

View reviewed changes

SakiTakamachi approved these changes Aug 18, 2024

View reviewed changes

ext/mysqlnd: support ER_CLIENT_INTERACTION_TIMEOUT

474141c

Appla force-pushed the mysqlnd_support_ER_CLIENT_INTERACTION_TIMEOUT branch from 292530d to eee6137 Compare August 22, 2024 10:19

[skip ci] doc: add mysqlnd error code changes to UPGRADING

62926ec

Appla force-pushed the mysqlnd_support_ER_CLIENT_INTERACTION_TIMEOUT branch from eee6137 to 62926ec Compare August 22, 2024 10:21

NEWS

971bdab

SakiTakamachi merged commit 555b603 into php:master Aug 26, 2024
7 of 10 checks passed

juslintek mentioned this pull request Sep 26, 2024

Mysql as gone away dunglas/frankenphp#290

Closed

	if (mysqli_get_server_version($link) < 80024) {
	if (mysqli_get_server_version($link) < 80024 \|\| str_contains(mysqli_get_server_info($link), 'MariaDB')) {

mysqlnd: support ER_CLIENT_INTERACTION_TIMEOUT #13618

mysqlnd: support ER_CLIENT_INTERACTION_TIMEOUT #13618

Conversation

Appla commented Mar 7, 2024

kamil-tekiela commented Mar 7, 2024

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

SakiTakamachi Mar 10, 2024 • edited Loading

Choose a reason for hiding this comment

Appla Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Appla commented Mar 8, 2024

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

Appla Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Appla Mar 9, 2024 • edited Loading

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

Choose a reason for hiding this comment

mbeccati commented Apr 11, 2024

bukka commented Apr 11, 2024

kamil-tekiela commented Apr 11, 2024

bukka commented Apr 11, 2024

mbeccati commented Apr 11, 2024

bukka commented Apr 11, 2024

bukka commented Apr 11, 2024

kamil-tekiela commented Apr 11, 2024

alexdowad commented Apr 15, 2024

iluuu1994 commented Apr 15, 2024

Appla commented Apr 22, 2024

mbeccati commented Apr 22, 2024

mbeccati commented Apr 22, 2024

Appla commented Apr 24, 2024

Appla commented Jun 17, 2024

mbeccati left a comment

Choose a reason for hiding this comment

mconkin commented Aug 7, 2024

mbeccati commented Aug 7, 2024

mconkin commented Aug 7, 2024

mbeccati commented Aug 14, 2024

bukka commented Aug 18, 2024

SakiTakamachi left a comment

Choose a reason for hiding this comment

SakiTakamachi commented Aug 19, 2024

SakiTakamachi Mar 10, 2024 •

edited

Loading

Appla Mar 11, 2024 •

edited

Loading

Appla Mar 11, 2024 •

edited

Loading

Appla Mar 9, 2024 •

edited

Loading