Skip to content

Fix Cancel #234

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Fix Cancel #234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e529adc
add cancel running query test and remove state lock in cancel method
Wraith2 Sep 29, 2019
27cf2e5
use delay wait in cancel tests to avoid hanging test runner
Wraith2 Oct 17, 2019
8016662
remove commented out test
Wraith2 Oct 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 25 additions & 25 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,41 +598,41 @@ internal void Cancel(object caller)
// Keep looping until we either grabbed the lock (and therefore sent attention) or the connection closes\breaks
while ((!hasLock) && (_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken))
Copy link

@Samirat Samirat Oct 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

If we don't get the lock, won't this loop / send attention multiple times? Is that intended?

{
// try to take the lock so that if another command is attempted it will queue on the lock
// but don't require that the lock be taken because otherwise attention cannot be sent
// during command execution causing cancellation to wait
// This lock is also protecting against concurrent close and async continuations
Monitor.TryEnter(this, _waitForCancellationLockPollTimeout, ref hasLock);
Copy link

@Samirat Samirat Oct 22, 2019

Choose a reason for hiding this comment

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

What's the point of taking a lock at all here? If we actually need to prevent new commands being started while sending attention, then we can't allow this lock to be optional, and if we don't, then why have it at all.

if (hasLock)
{ // Lock for the time being - since we need to synchronize the attention send.
// This lock is also protecting against concurrent close and async continuations

// Ensure that, once we have the lock, that we are still the owner
if ((!_cancelled) && (_cancellationOwner.Target == caller))
{
_cancelled = true;
// Ensure that that we are still the owner
if ((!_cancelled) && (_cancellationOwner.Target == caller))
{
_cancelled = true;

if (_pendingData && !_attentionSent)
if (_pendingData && !_attentionSent)
{
bool hasParserLock = false;
// Keep looping until we have the parser lock (and so are allowed to write), or the connection closes\breaks
while ((!hasParserLock) && (_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken))
{
bool hasParserLock = false;
// Keep looping until we have the parser lock (and so are allowed to write), or the connection closes\breaks
while ((!hasParserLock) && (_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken))
try
{
try
_parser.Connection._parserLock.Wait(canReleaseFromAnyThread: false, timeout: _waitForCancellationLockPollTimeout, lockTaken: ref hasParserLock);
Copy link

@Samirat Samirat Oct 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Feels like there should be an async version of this function. If the locks were async, wouldn't that also fix the cancellation hang?

if (hasParserLock)
{
_parser.Connection._parserLock.Wait(canReleaseFromAnyThread: false, timeout: _waitForCancellationLockPollTimeout, lockTaken: ref hasParserLock);
if (hasParserLock)
{
_parser.Connection.ThreadHasParserLockForClose = true;
SendAttention();
}
_parser.Connection.ThreadHasParserLockForClose = true;
SendAttention();
}
finally
}
finally
{
if (hasParserLock)
{
if (hasParserLock)
if (_parser.Connection.ThreadHasParserLockForClose)
{
if (_parser.Connection.ThreadHasParserLockForClose)
{
_parser.Connection.ThreadHasParserLockForClose = false;
}
_parser.Connection._parserLock.Release();
_parser.Connection.ThreadHasParserLockForClose = false;
}
_parser.Connection._parserLock.Release();
}
}
}
Expand Down
61 changes: 61 additions & 0 deletions src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,5 +282,66 @@ private static void TimeOutDuringRead(string constr)
throw;
}
}

[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
public static void CancelDoesNotWait()
{
const int delaySeconds = 30;
const int cancelSeconds = 1;

using (SqlConnection conn = new SqlConnection(s_connStr))
using (var cmd = new SqlCommand($"WAITFOR DELAY '00:00:{delaySeconds:D2}'", conn))
{
conn.Open();

Task.Delay(TimeSpan.FromSeconds(cancelSeconds))
.ContinueWith(t => cmd.Cancel());

DateTime started = DateTime.UtcNow;
DateTime ended = DateTime.MinValue;
Exception exception = null;
try
{
cmd.ExecuteNonQuery();
}
catch (Exception ex)
{
exception = ex;
}
ended = DateTime.UtcNow;

Assert.NotNull(exception);
Assert.InRange((ended - started).TotalSeconds, 0, delaySeconds - 1);
}
}

[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
public static async Task AsyncCancelDoesNotWait()
{
const int delaySeconds = 30;
const int cancelSeconds = 1;

using (SqlConnection conn = new SqlConnection(s_connStr))
using (var cmd = new SqlCommand($"WAITFOR DELAY '00:00:{delaySeconds:D2}'", conn))
{
await conn.OpenAsync();

DateTime started = DateTime.UtcNow;
Exception exception = null;
try
{
await cmd.ExecuteNonQueryAsync(new CancellationTokenSource(TimeSpan.FromSeconds(cancelSeconds)).Token);
}
catch (Exception ex)
{
exception = ex;
}
DateTime ended = DateTime.UtcNow;

Assert.NotNull(exception);
Assert.InRange((ended - started).TotalSeconds, cancelSeconds, delaySeconds - 1);
}
}

}
}