Request: Use MySqlConnector for async I/O

[bgrainger]

The MySqlDbHelper code uses ExecuteReaderAsync, etc.. It's a long-standing bug in MySql.Data (bug 70111) that async I/O is not implemented correctly; thus this method will actually run synchronously. (See also here and here on Stack Overflow.)

To fix this, you could switch to https://github.com/mysql-net/MySqlConnector, an OSS replacement for MySql.Data that supports true asynchronous I/O.

If you're interested in this, I'd be happy to open a PR.

[mikependon]

@rdagumampan - this is what I told you before :)

[mikependon]

@bgrainger - thanks for this. Please allow me to gather some information.

May I ask a question about MySqlConnector as well?

After you call the DbDataReader.ExecuteReader() method, do you also requiring NOT to dispose the DbCommand object when extracting the DbDataReader object values?

The other DB providers (ie: SqlConnection, SQLiteConnection and NpgsqlConnection) supports the DbCommand disposal even before extracting the DbDataReader values. However, the MySql.Data.MySqlConnection (DbCommand) does not.

This is why I have these code lines.

• https://github.com/mikependon/RepoDb/blob/53d64b47f4e187bcb34be6c2c30356dea844a2e6/RepoDb.MySql/RepoDb.MySql/DbSettings/MySqlDbSetting.cs#L21
• https://github.com/mikependon/RepoDb/blob/53d64b47f4e187bcb34be6c2c30356dea844a2e6/RepoDb.Core/RepoDb/Extensions/DbConnectionExtension.cs#L569
• https://github.com/mikependon/RepoDb/blob/53d64b47f4e187bcb34be6c2c30356dea844a2e6/RepoDb.Core/RepoDb/Extensions/DbConnectionExtension.cs#L656

[bgrainger]

That's a recent bug in Connector/NET (well, two years old now, I guess), that I've also documented at Stack Overflow.

MySqlConnector does not have this bug. It also fixes many other issues.

That reminds me, though: one possibly-breaking change is that MySqlConnector is more strict about transactions. It is simple to update code to be compliant, or to add a connection string option to disable strict transaction checking.

[bgrainger]

After a quick check, it looks like your code is pretty thorough about passing around the active transaction, so I expect you won't run into any transaction-related problems.

[mikependon]

Okay. Let me play around with it :) I will create a very small repository to replicate an issue I encountered at MySql.Data at MySqlConnector.

In anyway, thank you for this recommendation. We are now assessing. We will get back to you soon.

[mikependon]

Would you be able to contribute here RepoDb.MySqlConnector? I plan to introduce this one soon. No hurry for now.

[bgrainger]

Was that last comment directed to me? Are you asking for RepoDb.MySql to be duplicated as RepoDb.MySqlConnector?

[mikependon]

Yes, I can initially create the solutions and will eventually ask you if you could help me with that moving forward to have it work with MySqlConnector.

[mikependon]

@bgrainger - this is next on line for me. Any news on your side here?

[bgrainger]

Sorry, I lost track of this. 😬

I'm happy to contribute a PR if copy/pasting the "RepoDb.MySql" as "RepoDb.MySqlConnector" (then making appropriate changes) is still the preferred approach.

[mikependon]

@bgrainger - I am also thinking to combine your library inside the RepoDb.MySql and create the dedicated integration tests for MySqlConnector. Same goes with the RepoDb.SqlServer in which both MDS and SDS is present at one package. Your thoughts?

[bgrainger]

This should be possible, but may be complicated by the fact that both libraries (MySql.Data and MySqlConnector) use the same namespace: mysql-net/MySqlConnector#189. You'd have to use extern aliases to reference both libraries.

I'm not sure if this would cause any problems for consumers of the library, or if it would end up "just working" based on the specific MySQL library they referenced.

[bgrainger]

I pushed the changes here: bgrainger@e2bc342

They're simple enough, but unfortunately it breaks all the tests with hundreds of errors:

1>C:\Code\RepoDb\RepoDb.MySql\RepoDb.MySql.IntegrationTests\Operations\BatchQueryTest.cs(37,41,37,56): error CS0433: The type 'MySqlConnection' exists in both 'MySql.Data, Version=8.0.20.0, Culture=neutral, PublicKeyToken=c5687fc88969c44d' and 'MySqlConnector, Version=0.66.0.0, Culture=neutral, PublicKeyToken=d33d3e53aa5f8c92'

Even though the test project still only references MySql.Data 8.0.20, the transitive reference causes both libraries to be in scope, and the compiler can't decide which one to pick.

This would be painful to have to remedy (with extern aliases) in all consumers, so I think it rules out this approach until the namespace is changed.

Since a lot of the classes (e.g., the ones that have to end up referencing a specific MySqlDbType instance) ended up getting copied anyway, it might be best to go with the original plan of copying the entire library?

[mikependon]

Make sense! Thanks for exercising it. Then, I would be very happy if you do the entire copy and make a separate "RepoDb.MySqlConnector" folder as part of the top folder (pushed by you). Then, let us work together to pass all the things there.

Would it be possible for you to initiate the copy of the entire folder of "RepoDb.MySql" and create a new folder for "RepoDb.MySqlConnector"?

[bgrainger]

Yes, I can do that.

[mikependon]

Nice if you can make your own branch then I will also push my updates there until we finalize the solution and merge it back to the master. I will add you as collaborator on the project.

Also, I will put you as owner of RepoDb.MySqlConnector Nuget Package if you are fine with that.

[bgrainger]

I've pushed a new branch, mysqlconnector. To avoid it just being a wall of green newly-added code, I split it into two commits. The first just duplicated all the RepoDb.MySql files as-is; the second shows the actual changes: fa13702

[mikependon]

I had reviewed all your changes. It is awesome and well-refined! :) I will revisits your branch on Saturday and will ensure running all the Unit/Integration Tests on my local DEV environment. Once they're all passing, then let us merge it right away.

P.S: Just do all the changes if you think you still have something before Saturday so I can include those on my testing and validation activities.

Big thanks for this "change" mate, you did all my work by integrating your library to RepoDb.

[mikependon]

@bgrainger - I made a reviews today on your solution. Also, I had run the Integration Tests locally and it is all green. Thanks and cheers for this branch.

Would you be able to issue a PR anytime soon? I will then create the dedicated AppVeyor CI builds for this project and relate it to its README.

Integration and Unit Tests result:

[bgrainger]

If you're happy with the mysqlconnector branch (looks like you pushed a few updates to it), I can open a PR from it: #446

[mikependon]

I made a thorough review today CET. Yes, you can now issue a PR so I can finalize the CI builds and the docs this weekend.

[bgrainger]

Opened here: #446

[mikependon]

@bgrainger - done. Feel free to add your library on the Credits section of the Main README of the project.

[bgrainger]

Thanks for merging it! Done: c8d0f77.

[mikependon]

I made an additional series of code-checks and local tests execution (also with App Veyor CI Builds) today.

The initial release can be found here.

The documentation has been updated to the latest.