-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Correct timeout remarks for SqlClient async command methods #3343
Conversation
Test used to confirm the behavior: using var conn = new SqlConnection("...");
using var cmd = new SqlCommand("WAITFOR DELAY '00:00:05'", conn) { CommandTimeout = 1 };
conn.Open();
// Sync: times out
cmd.ExecuteNonQuery();
// Async: times out
await cmd.ExecuteNonQueryAsync();
// Old-style async: does not time out
var asyncResult = cmd.BeginExecuteNonQuery();
asyncResult.AsyncWaitHandle.WaitOne();
var result = cmd.EndExecuteNonQuery(asyncResult); |
a3e8317
to
ef0106e
Compare
Corresponding PR for Microsoft.Data.SqlClient: dotnet/SqlClient#264 |
@carlossanlop the SqlClient people have approved this, and the corresponding changes have already been merged in https://github.com/dotnet/sqlclient. Can someone sign off on this from your side as well? |
@BillWagner @mairaw can someone in your team please take a look? |
Sure, I'll review this one. @stevestein you should keep an eye on PRs assigned to you for review for the System.Data space. |
ef0106e
to
dc58aca
Compare
ping, this is still waiting validation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay @roji. Thanks for making this change. It looks good. I'll merge it.
No problem at all, thanks @carlossanlop! |
After investigating npgsql/npgsql#2457, it turns out that SqlCommand.CommandTimeout works well with async methods, just not with the old-style pre-async/await Begin* methods. However, the documentation remarks still indicate that timeouts aren't supported with any async SqlClient APIs.