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 underlying assumption of SessionTrack that not work well for MariaDB #603

Closed
wants to merge 1 commit into from

Conversation

elemount
Copy link
Contributor

@elemount elemount commented Jan 15, 2019

The implies ProtocolCapabilities.SessionTrack is not correct for MariaDB, when MariaDB is behind a proxy like MaxScale.
MariaDB take such action

  1. See the client support SessionTrack or not
  2. If not, just set the flag SessionStateChanged to true and then write noting on the the packet. See https://github.com/MariaDB/server/blob/10.2/sql/protocol.cc#L272

So if MariaDB is under a proxy(proxy does not set the capacity), it will throw such exception

MySql.Data.MySqlClient.MySqlException
  HResult=0x80004005
  Message=Failed to read the result set.
  Source=MySqlConnector
  StackTrace:
   at MySql.Data.MySqlClient.MySqlDataReader.ActivateResultSet(ResultSet resultSet) in C:\projects\mysqlconnector\src\MySqlConnector\MySql.Data.MySqlClient\MySqlDataReader.cs:line 90
   at MySql.Data.MySqlClient.MySqlDataReader.<ReadFirstResultSetAsync>d__92.MoveNext() in C:\projects\mysqlconnector\src\MySqlConnector\MySql.Data.MySqlClient\MySqlDataReader.cs:line 298
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at MySql.Data.MySqlClient.MySqlDataReader.<CreateAsync>d__91.MoveNext() in C:\projects\mysqlconnector\src\MySqlConnector\MySql.Data.MySqlClient\MySqlDataReader.cs:line 289
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at MySqlConnector.Core.TextCommandExecutor.<ExecuteReaderAsync>d__1.MoveNext() in C:\projects\mysqlconnector\src\MySqlConnector\Core\TextCommandExecutor.cs:line 37
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at MySql.Data.MySqlClient.MySqlCommand.<ExecuteNonQueryAsync>d__60.MoveNext() in C:\projects\mysqlconnector\src\MySqlConnector\MySql.Data.MySqlClient\MySqlCommand.cs:line 261
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at MySql.Data.MySqlClient.MySqlCommand.ExecuteNonQuery() in C:\projects\mysqlconnector\src\MySqlConnector\MySql.Data.MySqlClient\MySqlCommand.cs:line 62
   at mysqlconnectortest.Program.Main(String[] args) in C:\Users\shuodl\source\repos\mysqlconnectortest\mysqlconnectortest\Program.cs:line 19

Inner Exception 1:
IndexOutOfRangeException: Index was outside the bounds of the array.

Here is a workaround for it.

@elemount elemount force-pushed the master branch 2 times, most recently from 7411385 to f21b4ea Compare Jan 15, 2019
@bgrainger
Copy link
Member

bgrainger commented Jan 15, 2019

So, in the initial handshake, MariaDB sends CLIENT_SESSION_TRACK in the capabilities flags, but MaxScale doesn't set that flag in its initial handshake (that is sent to the client connecting to the proxy)? Thus, MySqlConnector doesn't set that flag in the handshake response packet, and MariaDB thinks the client doesn't support session track.

Right now, MySqlConnector follows the advice in capability negotiation:

The client should only announce the capabilities in the Handshake Response Packet that it has in common with the server.

It seems like the underlying bug is MySqlConnector's incorrect handling of the OK Packet. The state change data may only be read if the CLIENT_SESSION_TRACK flag was negotiated, but the OkPayload parsing code just proceeds as if that's always the case.

We need to save whether the server reported the availability of CLIENT_SESSION_TRACK, then change how OK is parsed.

@bgrainger bgrainger closed this in 17ad067 Jan 15, 2019
@bgrainger
Copy link
Member

bgrainger commented Jan 15, 2019

@elemount Please let me know if the fix shipped in 0.49.1 resolves this problem for you. (I didn't set up a MaxScale proxy for testing.)

@elemount
Copy link
Contributor Author

elemount commented Jan 16, 2019

@bgrainger , Thank you so much. I tested the 0.49.1 and it resolved the problem for me.
But I noticed in your fix, there is a little strange code, not sure if it is a type mistake
var ok = OkPayload.Create(payload.AsSpan(), Session.SupportsDeprecateEof, Session.SupportsDeprecateEof);.

@bgrainger
Copy link
Member

bgrainger commented Jan 16, 2019

That is a typo and definitely not the change I meant to make. I'll release another version with the fixed code. Thanks for spotting it!

@bgrainger
Copy link
Member

bgrainger commented Jan 16, 2019

@elemount 0.49.2 is now available, and has the final fix for this issue.

@elemount
Copy link
Contributor Author

elemount commented Jan 17, 2019

@bgrainger , thank you. I've verified the 0.49.2 still works for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants