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

MysqlMutext component fix same connection but difference database fail #19603

Merged

Conversation

kamarton
Copy link
Contributor

@kamarton kamarton commented Oct 3, 2022

Q A
Is bugfix? ✔️
New feature? ✔️
Breaks BC?
Fixed issues #19316

framework/mutex/MysqlMutex.php Show resolved Hide resolved
@samdark samdark added this to the 2.0.47 milestone Oct 4, 2022
@bizley bizley merged commit 0b4741e into yiisoft:master Oct 6, 2022
@rhertogh
Copy link
Contributor

This PR is not backwards compatible. If an application would, for example, use different databases for each of it's customers a.k.a. tenants, the mutex would no longer work on the MySQL server level after the implementation of this PR.

I think the assumption made in #19316 is incorrect. Different databases should not matter for the mutex, it's the driver for the mutex that determines its scope. So as long as the same endpoint is used the application should be able to count on the mutex behavior of that driver (in this case the MySQL server).

If an application needs 2 different scopes for different mutexes as in #19316 the keyPrefix could be configured.
The current implementation forces the database name into the key (https://github.com/yiisoft/yii2/pull/19603/files#diff-4c57428f9b73c50ff9f17f5f236aac5447feddfbbd34c6dc467ba5d8e06583c4R55-R57) therefore there is no longer any way to rely on the mutex for any "global" purposes, unless the keyPrefix would be set in those cases (which would make it backwards incompatible).

@bizley
Copy link
Member

bizley commented Oct 18, 2022

I'm not sure if this reasoning is right, are you talking about different connections + different dbs or one connections + different dbs because the one case should be fine here. Or should not?

@rhertogh
Copy link
Contributor

@bizley As far as I know both the connection or the database should not make any difference.

By default the "scope" of the mutex is "server wide" since the MySQL server itself stores the lock in the "performance_schema" database in the "metadata_locks" table (https://dev.mysql.com/doc/refman/8.0/en/locking-functions.html). Therefore any client, no matter the database (you can even open up a CLI client without using any database), would request the same lock (assuming they use the same name for the lock).

So if the default behavior is a "server wide" scope for the uniqueness of a mutex lock this PR would brake that default behavior (and therefore braking backwards compatibility).

@rhertogh
Copy link
Contributor

My suggestion would be to remove the default value for the keyPrefix (https://github.com/yiisoft/yii2/pull/19603/files#diff-4c57428f9b73c50ff9f17f5f236aac5447feddfbbd34c6dc467ba5d8e06583c4R55-R57).
If needed it then can be set in the configuration, e.g.:

'mutex1' => [
    'class' => 'yii\mutex\MysqlMutex',
    'db' => 'db1',
    'keyPrefix' => 'myPrefix1'
],
'mutex2' => [
    'class' => MysqlMutex::class,
    'db' => 'db2',
    'keyPrefix' => 'myPrefix2'
],

@samdark You mentioned before that the name should be included (#19316 (comment)), what are your thoughts on this (please see comments above)?

@rob006
Copy link
Contributor

rob006 commented Oct 25, 2022

@rhertogh The problem is that old behavior is unexpected and hard to debug. If you share DB server with others, you may not even be aware the problem, you just get random lock failures out of nowhere. So while this is a BC break, I think that new behavior is more sensible as a default. But this change indeed should be documented in UPGRADE.md.

@rhertogh
Copy link
Contributor

rhertogh commented Oct 25, 2022

@rob006 True, a note in UPGRADE.md could cover it.
It should also include the remark that a database name has to be specified in the database connection, a "generic" connection (without any specific database name) would otherwise fail with ERROR 3057 (42000): Incorrect user-level lock name 'NULL'.
Or the keyPrefix has to be specified for the mutex.

Another option would be to change the default value from $this->keyPrefix = new Expression('DATABASE()');
to $this->keyPrefix = new Expression('COALESCE(DATABASE(), "")');
so it can deal with connections without a database name;

@samdark
Copy link
Member

samdark commented Oct 26, 2022

@rhertogh do you have time for a pull request with additions to UPGRADE?

rhertogh added a commit to rhertogh/yii2 that referenced this pull request Oct 27, 2022
Added UPGRADE.md note since the default "scope" of the yii\mutex\MysqlMutex has changed.
@rhertogh
Copy link
Contributor

@samdark I've created a PR: #19645
During the writing I recognized another important point to consider, please see the "warning" in the UPGRADE.md (https://github.com/yiisoft/yii2/pull/19645/files#diff-2632bce37f2b1a645fe1a8909903481e3c4ca921d46cd9bf589752b4a3c7086eR78)

samdark pushed a commit that referenced this pull request Nov 7, 2022
Added UPGRADE.md note since the default "scope" of the yii\mutex\MysqlMutex has changed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants