MySqlConnector icon indicating copy to clipboard operation
MySqlConnector copied to clipboard

Transaction isolation level should not default to `REPEATABLE READ` regardless of server configuration if left unspecified

Open bdach opened this issue 1 year ago • 3 comments
trafficstars

Software versions MySqlConnector version: 2.1.6, but still relevant on latest master as far as I can tell Server type (MySQL, MariaDB, Aurora, etc.) and version: Not relevant .NET version: Not relevant

Describe the bug If a transaction is started without specifying an isolation level, it will silently default to IsolationLevel.Unspecified, which is really just REPEATABLE READ:

https://github.com/mysql-net/MySqlConnector/blob/999a3ab913a0112b5819f57f603b109e9c435e6f/src/MySqlConnector/MySqlConnection.cs#L155-L156

The comment is correct in stating that REPEATABLE READ is the default innodb transaction isolation level, however, this is confusing in cases where the global transaction isolation level on the mysql instance that queries are directed to is deliberately changed from the configuration default.

Related: https://github.com/mysql-net/MySqlConnector/issues/772

Exception Not applicable

Code sample See usage. On a mysql transaction with READ COMMITTED default transaction isolation level this would instead force REPEATABLE READ and as such caused a long investigation into ensuing deadlocks, that was "fixed" by https://github.com/ppy/osu-queue-score-statistics/commit/5c23e0f02b67c6089922ea553ce03894f2690011.

Expected behavior MySqlConnector does not attempt to explicitly set any transaction isolation level when starting a transaction with IsolationLevel.Unspecified.

Additional context Not applicable

bdach avatar Jan 26 '24 13:01 bdach

I think this comment explains the current behaviour: https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/issues/1043#issuecomment-599817149

It's "a" default level, but there's absolutely no guarantee that the server hasn't been configured with a different default level. So it should be issued every time so that ADO.NET semantics are followed, regardless of server defaults.

And https://learn.microsoft.com/en-us/dotnet/api/system.data.common.dbconnection.begintransaction?view=net-8.0#remarks:

If you do not specify an isolation level, the default isolation level for the specific type of connection is used.

IMO, the documentation doesn't state or imply that an implicit server-set default will be used if not specified by the client.

I'm a little reticent to change this because

  • it's been this way for a very long time (since 783645f52d31c78edd0800cbf7ce7d3917544082) and would be a silent breaking change for almost all users of the library
  • it has a very simple workaround: explicitly specify the isolation level you want

bgrainger avatar Jan 26 '24 14:01 bgrainger

I can understand that this would be a pretty big API break and the use case is likely niche, but I hope it is also understandable that the current behaviour results in a rather unexpected and confusing outcome. Maybe at least a remark in documentation could be added for posterity?

bdach avatar Jan 26 '24 15:01 bdach

Yes, this should certainly be clearly documented.

FWIW, MySql.Data also defaults to REPEATABLE READ: https://github.com/mysql/mysql-connector-net/blob/0bb0e8c4d9f327138e758b6a31c3d8ebb70a2eae/MySQL.Data/src/MySqlConnection.cs#L490-L492

bgrainger avatar Jan 26 '24 15:01 bgrainger