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

Handle QueryInterrupted exceptions when canceling completed queries. #248

Merged
merged 3 commits into from Apr 22, 2017

Conversation

adampoit
Copy link
Contributor

@adampoit adampoit commented Apr 21, 2017

KILL QUERY will interrupt a subsequent query if the command it was intended to cancel has already completed, and the connection is idle. In order to handle this case, we issue a dummy query to catch the QueryInterrupted exception. See https://bugs.mysql.com/bug.php?id=45679.

I ran into this issue while using Dapper's QueryImpl method. If reader.FieldCount == 0, then Dapper will try to cancel the query. Upon the next command issued by the connection, it is immediately interrupted.

Generally, using Query without selecting any columns is wrong, but this would be a problem for any query that completed before it was canceled. The MySQL.Data implementation from Oracle handles both of these cases without an error.

KILL QUERY will interrupt a subsequent query if the command it was intended to cancel has already completed, and the connection is idle. In order to handle this case, we issue a dummy query to catch the QueryInterrupted exception. See https://bugs.mysql.com/bug.php?id=45679.
// KILL QUERY will kill a subsequent query if the command it was intended to cancel has already completed.
// In order to handle this case, we issue a dummy query to catch the QueryInterrupted exception.
// See https://bugs.mysql.com/bug.php?id=45679
var killClearCommand = new MySqlCommand("SELECT * FROM fake_table LIMIT 0;", connection);
Copy link
Member

@bgrainger bgrainger Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try something like DO SLEEP(0.001) to clear the pending cancellation?

Copy link
Member

@bgrainger bgrainger Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DO SLEEP(0) is even better (for some reason I was assuming it had to be a non-zero wait, but SLEEP(0) seems to work fine).

Copy link
Contributor Author

@adampoit adampoit Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -84,6 +84,8 @@ public void DoCancel(MySqlCommand commandToCancel, MySqlCommand killCommand)
// blocking the other thread for an extended duration.
killCommand.CommandTimeout = 3;
killCommand.ExecuteNonQuery();

commandToCancel.IsCanceled = true;
Copy link
Member

@bgrainger bgrainger Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this IsCanceled property should be combined with the existing MySqlSession.m_state so that all state related to cancellation is in one place.

Copy link
Member

@bgrainger bgrainger Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to handle this situation in MySqlSession.FinishQuerying; I can push that change.

Copy link
Contributor Author

@adampoit adampoit Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started messing around with the states, but if you know how you want to tackle this I have Allow edits from maintainers turned on, so go for it.

@@ -228,6 +228,18 @@ public void DapperQueryMultiple()
}
}

[Fact]
public async Task CancelCompletedCommand()
Copy link
Member

@bgrainger bgrainger Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not getting any kind of failure from this test when running it without the new code (so it's difficult to verify that the problem is fixed).

Copy link
Contributor Author

@adampoit adampoit Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What version of MySQL are you testing with? I tested this on 5.6.29. According to a duplicate of the bug I linked, the issue may be

repeatable with server version 5.1/5.5/5.6 and NOT REPEATABLE with server version 5.0 and 5.7

Copy link
Member

@bgrainger bgrainger Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using 5.7.

I still can't repro a failure with this test with 5.6, although I can get an unhandled exception when using Dapper's Query<int> followed by Execute.


using (await cmd.ExecuteReaderAsync().ConfigureAwait(false))
cmd.Cancel();
}
Copy link
Member

@bgrainger bgrainger Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be followed by a second command that will be the unintentional victim of cancellation?

Copy link
Contributor Author

@adampoit adampoit Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Dispose method should be executing the second command, but perhaps it would be better to be explicit.

Copy link
Member

@bgrainger bgrainger Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried SELECT 2; but that doesn't repro the problem, as per bullet 2 of bug 45679. So I think the INSERT/UPDATE combination of commands is a good one.

@bgrainger bgrainger merged commit fee93b1 into mysql-net:master Apr 22, 2017
2 checks passed
bgrainger added a commit that referenced this issue Apr 22, 2017
Handle QueryInterrupted exceptions when canceling completed queries.
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