Skip to content

Try/catch ResetConnectionAsync (works around #208) #328

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

Merged

Conversation

bnabholz
Copy link
Contributor

When the ConnectionPool is asked for a session, depending on server version, it will attempt to reauthenticate. On occasion the server will close the connection and the next read from the stream will be 0 bytes, causing the connection to fail. This PR wraps ResetConnectionAsync in a try/catch, and if it throws it will fall back to instantiating a new MySqlSession.

I'm not sure if we should be trying to catch only certain exceptions (EndOfStreamException?) and let others bubble up as usual.

@bgrainger
Copy link
Member

I'm not sure if we should be trying to catch only certain exceptions (EndOfStreamException?) and let others bubble up as usual.

I generally prefer to catch the specific exception I know I'm handling; however, in this case it seems good to fall back to creating a new session if any kind of IOException or SocketException is thrown.

@bgrainger bgrainger merged commit 0851817 into mysql-net:master Sep 13, 2017
@bnabholz bnabholz deleted the ignore-resetconnection-exceptions branch September 13, 2017 16:35
@bgrainger
Copy link
Member

I'm going to update the code I just pushed to only catch EndOfStreamException, IOException, SocketException (same as TryPingAsync); I don't want other programming errors to be masked by this catch handler (as it would dramatically reduce performance by essentially disabling connection pooling).

@bnabholz
Copy link
Contributor Author

Sounds good to me. Thanks for reviewing and thanks to you and the other contributors for this project!

@bgrainger
Copy link
Member

Shipped in 0.26.4; thanks for the contribution!

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.

2 participants