Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix MySQL build problems with ConnectionException detection #3656

Closed
wants to merge 1 commit into from

Conversation

mmucklo
Copy link
Contributor

@mmucklo mmucklo commented Aug 17, 2019

Q A
Type improvement
BC Break no
Fixed issues #3655

Summary

Over the past few weeks MySQL builds have been randomly failing due to the following sorts of errors (happened with both MySQLi and PDO):

There was 1 failure:
752
7531) Doctrine\Tests\DBAL\Functional\ExceptionTest::testConnectionException with data set #0 (array('not_existing'))
754Failed asserting that exception of type "Doctrine\DBAL\Exception\DriverException" matches expected exception "Doctrine\DBAL\Exception\ConnectionException". Message was: "An exception occurred in driver: The server requested authentication method unknown to the client" at
755/home/travis/build/doctrine/dbal/lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php:71
756/home/travis/build/doctrine/dbal/lib/Doctrine/DBAL/Driver/Mysqli/Driver.php:16
757/home/travis/build/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:354
758/home/travis/build/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:418
759/home/travis/build/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:378
760/home/travis/build/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:322
761/home/travis/build/doctrine/dbal/tests/Doctrine/Tests/DBAL/Functional/ExceptionTest.php:369

This is happening because the test is expecting a ConnectionException, but instead a DriverConnection is incorrectly happening.

MySQLi driver flaky failed builds:
https://travis-ci.org/doctrine/dbal/jobs/565778380
https://travis-ci.org/doctrine/dbal/jobs/569139933
https://travis-ci.org/doctrine/dbal/jobs/569146693

MySQL PDO driver flaky failed builds:
https://travis-ci.org/doctrine/dbal/jobs/569146680
https://travis-ci.org/doctrine/dbal/jobs/569146690

@morozov
Copy link
Member

morozov commented Aug 20, 2019

The server requested authentication method unknown to the client

Not sure how the fix addresses the issue. It masks the authentication method issue behind the same exception class as is expected as a result of login failure.

@mmucklo
Copy link
Contributor Author

mmucklo commented Aug 20, 2019

@morozov

The test that fails every time tries to connect to a bogus hostname, and makes sure that a ConnectionException happens (see the sample trace below).

Perfectly reasonable, except that the code does not trap all possible MySQL connection error states.

Specifically the MySQL client code will seemingly for no good reason return a 2006 or a 2054 error some times even if trying to connect to a bogus hostname.

2006 - The MySQL server has gone away

This can seem to happen randomly trying to connect to a bogus hostname.

This can happen in mid request also, which wouldn't be a ConnectionException, but how to distinguish 2006 during a Connection from 2006 during a regular request?

Hence the subclassing.

2054 - The server requested authentication method unknown to the client

This is also strange because there's no MySQL host to connect to, yet the driver apparently returns a 2054 error sometimes (as you can see from the tests referenced above).

NOTE: this is a different case then the MySQL 8.0 connection issue when the default_authentication method isn't properly set (which was already earlier fixed in the docker command). This new occurrence happens when there isn't even a MySQL server to connect to.

2054 is a new previously uncaught error number in the code base, but only seems to happen on Connect, hence it seems safe to classify both cases as a ConnectionException.

This case does not require the subclass like the other does.

Sample trace (PDO version):

/home/travis/build/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php:31
785/home/travis/build/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOMySql/Driver.php:22
786/home/travis/build/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:354
787/home/travis/build/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:418
788/home/travis/build/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:378
789/home/travis/build/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:322
790/home/travis/build/doctrine/dbal/tests/Doctrine/Tests/DBAL/Functional/ExceptionTest.php:369

@morozov
Copy link
Member

morozov commented Aug 21, 2019

The test that fails every time tries to connect to a bogus hostname, and makes sure that a ConnectionException happens (see the sample trace below).

As far as I understand from the error message:

  1. Doctrine\Tests\DBAL\Functional\ExceptionTest::testConnectionException with data set #0 (array('not_existing'))
    Failed asserting that exception of type "Doctrine\DBAL\Exception\DriverException" matches expected exception "Doctrine\DBAL\Exception\ConnectionException". Message was: "An exception occurred in driver: SQLSTATE[HY000] [2054] The server requested authentication method unknown to the client"

The test is trying to connect to a valid server as a non-existing user and get an error like 1045 (Access denied for user 'not_existing') which is later expected to be converted to a ConnectionException. Instead, sometimes it gets a different error response and fails. By converting the 2006 and 2054 error codes into the same exception and not changing the test, you're masking the existing problem. If you rework the test to expect a specific error code, it will keep failing as it does now because this patch doesn't fix the root cause of the issue.

@morozov
Copy link
Member

morozov commented Aug 21, 2019

Unless it is clear why MySQL 8 doesn't respect the --default-authentication-plugin=mysql_native_password configuration or, most likely, replies something which the client cannot parse in the case when the user doesn't exist, the best course of action seems to mark this test incomplete for MySQL 8.

@mmucklo
Copy link
Contributor Author

mmucklo commented Aug 21, 2019

@morozov

On second review, yes, you are correct, all the failures I provided are cases of trying to connect with a user that is non_existing (as that's what's passed in from the list of getConnectionParams()).

I think breaking this apart into two different PRs (one for each error number) might make the conversation easier, as I would plead the case still that perhaps 2054 should be classified as a ConnectionException, and 2006 sometimes.

Otherwise marking the test as invalid for MySQL 8.0 might be the other possible order of business as you alluded to.

@mmucklo mmucklo closed this Aug 21, 2019
@morozov
Copy link
Member

morozov commented Aug 21, 2019

It looks like a MySQL bug to me. I can reproduce it as follows:

  1. Start a MySQL 8 container:

    docker run \
        --rm \
        -e MYSQL_ALLOW_EMPTY_PASSWORD=yes \
        -p 33306:3306 \
        mysql:8.0 \
        --default-authentication-plugin=mysql_native_password
  2. When the server is ready, run the following script (PHP 7.3.8):

    $conn = mysqli_connect('127.0.0.1:33306', 'root', '', 'mysql');
    
    if (!$conn) {
        exit(1);
    }
    
    $query = 'SELECT VERSION()';
    
    $result = mysqli_query($conn, $query);
    
    if (!$result) {
        echo mysqli_error($conn), PHP_EOL;
        exit(1);
    }
    
    echo mysqli_fetch_row($result)[0], PHP_EOL;

    The expected output is something like 8.0.17

  3. Change the username in the connection parameters to something invalid. Re-run the script. The expected output is:

    Warning: mysqli_connect(): (HY000/1045): Access denied for user 'anonymous'@'172.17.0.1' (using password: YES)

    The actual output is:

    Warning: mysqli_connect(): The server requested authentication method unknown to the client [caching_sha2_password]

    Warning: mysqli_connect(): (HY000/2054): The server requested authentication method unknown to the client

    At the same time, the server reports its configuration as configured:

    mysql> show variables like 'default_authentication_plugin';
    +-------------------------------+-----------------------+
    | Variable_name                 | Value                 |
    +-------------------------------+-----------------------+
    | default_authentication_plugin | mysql_native_password |
    +-------------------------------+-----------------------+
    1 row in set (0.01 sec)
    

@mmucklo
Copy link
Contributor Author

mmucklo commented Aug 21, 2019

@morozov,

Would you consider 2054 to always be classified as a DriverException then? As that’s what the fall through is (for example say you didn’t set the authentication method correctly).

@morozov
Copy link
Member

morozov commented Aug 21, 2019

Aren't all exceptions produced by AbstractMySQLDriver::convertException() DriverExceptions or its subtypes regardless of the code?

@Ocramius
Copy link
Member

Related: #3662

@Ocramius Ocramius added this to the 2.10.0 milestone Aug 24, 2019
@morozov morozov removed this from the 2.10.0 milestone Nov 2, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants